Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor history cache to its own package #3601

Merged
merged 7 commits into from
Dec 6, 2022

Conversation

mindaugasrukas
Copy link
Contributor

@mindaugasrukas mindaugasrukas commented Nov 15, 2022

What changed?
Refactored history cache to its own package. Together Refactored DeleteManager to its package to avoid circular imports.

Why?
Code preparation for: #3594

How did you test it?
Unit tests, CI, make bins

Potential risks
Low risk as unit tests and builds are passing; this is just package renaming—no logic change.

Is hotfix candidate?
None

Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz... can it be called workflow cache, instead of history cache as it's caching workflow mutable state instead of workflow history events? All the variable names are also workflowCache. And I remember there're some refactor before (either in temporal or cadence repo) to change the name from historyCache to workflow cache.

Is it possible to create the new package under workflow? Like service/history/workflow/cache?

@mindaugasrukas
Copy link
Contributor Author

Plz... can it be called workflow cache, instead of history cache as it's caching workflow mutable state instead of workflow history events? All the variable names are also workflowCache. And I remember there're some refactor before (either in temporal or cadence repo) to change the name from historyCache to workflow cache.

Is it possible to create the new package under workflow? Like service/history/workflow/cache?

Sounds good. I'm just curious about, why workflow is under history (service/history/workflow). Is it HistoryWorkflow and HistoryWorkflowCache?

service/history/api/consistency_checker.go Outdated Show resolved Hide resolved
service/history/cache/fx.go Outdated Show resolved Hide resolved
@alexshtin
Copy link
Member

Sounds good. I'm just curious about, why workflow is under history (service/history/workflow). Is it HistoryWorkflow and HistoryWorkflowCache?

history here is related to service -- it is "history service". And all packages under it are used by this history service only.

archiveIfEnabled bool,
stage *tasks.DeleteWorkflowExecutionStage,
) error
}

DeleteManagerImpl struct {
shard shard.Context
historyCache Cache
workflowCache wcache.Cache
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

@@ -68,20 +66,20 @@ type (
}

ActivityReplicatorImpl struct {
historyCache workflow.Cache
workflowCache wcache.Cache
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

@@ -113,7 +113,7 @@ type (
historySerializer serialization.Serializer
metricsHandler metrics.MetricsHandler
namespaceRegistry namespace.Registry
historyCache workflow.Cache
workflowCache wcache.Cache
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

@@ -66,7 +67,7 @@ type (
controller *gomock.Controller
mockShard *shard.ContextTest
mockEventCache *events.MockCache
mockHistoryCache *workflow.MockCache
mockWorkflowCache *wcache.MockCache
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

@@ -154,7 +154,7 @@ type (
transactionMgrImpl struct {
shard shard.Context
namespaceRegistry namespace.Registry
historyCache workflow.Cache
workflowCache wcache.Cache
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

@@ -82,7 +82,7 @@ type (
namespaceRegistry namespace.Registry
clusterMetadata cluster.Metadata
executionMgr persistence.ExecutionManager
historyCache workflow.Cache
workflowCache wcache.Cache
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

@@ -89,7 +89,7 @@ func newQueueProcessorBase(
options *QueueProcessorOptions,
queueProcessor common.Daemon,
queueAckMgr queueAckMgr,
historyCache workflow.Cache,
wcache wcache.Cache,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

@@ -22,9 +22,9 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

//go:generate mockgen -copyright_file ../../../LICENSE -package $GOPACKAGE -source $GOFILE -destination cache_mock.go
//go:generate mockgen -copyright_file ../../../../LICENSE -package $GOPACKAGE -source $GOFILE -destination cache_mock.go
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

)

type (
historyCacheSuite struct {
workflowCacheSuite struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

@@ -105,7 +106,7 @@ func (s *WorkflowTaskHandlerCallbackSuite) SetupTest() {
s.mockEventsCache.EXPECT().PutEvent(gomock.Any(), gomock.Any()).AnyTimes()
s.logger = mockShard.GetLogger()

historyCache := workflow.NewCache(mockShard)
workflowCache := wcache.NewCache(mockShard)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

@mindaugasrukas mindaugasrukas merged commit c94d2bf into temporalio:master Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants