Move shard.Engine interface into history/interfaces package #7043
Conversation
service/history/common/engine.go
Outdated
| @@ -24,7 +24,7 @@ | |||
|
|
|||
| //go:generate mockgen -copyright_file ../../../LICENSE -package $GOPACKAGE -source $GOFILE -destination engine_mock.go | |||
|
|
|||
| package shard | |||
| package common | |||
There was a problem hiding this comment.
Note: initially I want to move it to something like "internal/common" or "internal/interfaces". But we can't do this - this is used in functional tests, and we technically put functional tests into separate "package"
I'd like to be part of the discussion as well.
Also want to understand what the structure will eventually look like, plans for other interfaces, MutableState, Context, ShardContext etc. To me, it feels like this is a big refactoring project and need some doc describing the overall plan. |
service/history/common/engine.go
Outdated
| @@ -24,7 +24,7 @@ | |||
|
|
|||
| //go:generate mockgen -copyright_file ../../../LICENSE -package $GOPACKAGE -source $GOFILE -destination engine_mock.go | |||
|
|
|||
| package shard | |||
| package common | |||
There was a problem hiding this comment.
You can call this historyinterfaces to avoid the need to disambiguate the imports and have a name that better conveys the purpose of the package.
There was a problem hiding this comment.
I think about that, but ultimately decide against it.
'interfaces` is better.
- it is/will be consistent.
- purpose is clear
- no redundant info (
historyis already part ofimport ".../history/...") - name is at least shorter
There was a problem hiding this comment.
No strong opinion on the new structure, mostly the naming.
Agree with @yycptt that it'd be better to understand the end goal before going to the implementation.
I think our problem is that we're going against Go best practices and defining interfaces on the producer side instead of the consumer.
If we stop using mocks and the pattern of interface + Impl, we wouldn't need to change the structure.
"one step at a time". I don't think we can avoid interfaces altogether (I also think we shoulnd't, but that is another story). |
Short term goal - better code structure, better ownership hierarchy, remove access to persistence layer, remove not needed dependencies. |
ychebotarev
force-pushed
the
y_ref2
branch
2 times, most recently
from
e1d36f1 to
a3ad883
Compare
ychebotarev
requested review from
stephanos and
alexshtin
and removed request for
bergundy and
alexshtin
alexshtin
changed the title
|
After all back and forth and offline discussions, I am ready to approve this. |
What changed?
"Engine" interface was moved to "history/common" module
Why?
Part of bigger refactoring efforts.
Goal is to reduce code entanglement, by separating interfaces and implementation, for some major components.
Currently, in lots of cases, interface and implementation are in a single file. This bloats the dependency graph, and introduce bad practices when dealing with "module cyclic dependency" error.
How did you test it?
unit tests