-
Notifications
You must be signed in to change notification settings - Fork 818
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
Implement Size() measuring size estimation for execution cache #6681
base: master
Are you sure you want to change the base?
Conversation
…mpty size estimation function for mutableState and shardContext
79e5291
to
b7ca226
Compare
|
||
// SetupTest runs before each test function in the suite. | ||
func (suite *WorkflowIdentifierTestSuite) SetupTest() { | ||
// Any necessary setup would go here, but it's not required in this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think, for greenfield stuff we prefer not to use suite code anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will change that to regular unit tests.
@@ -236,5 +236,7 @@ type ( | |||
|
|||
GetHistorySize() int64 | |||
SetHistorySize(size int64) | |||
|
|||
Size() int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest putting this in a distinct interface and composing them:
eg:
// in a common place, I guess common/size/est.go
might work, I don't feel strongly, but a place where it, but you may need to put associated stuff in a place where it can be referenced by all packages, so its own package in common seems reasonable.
// the general set of things you need to care about for size. In this case I guess it's just a single getter
type EstSize interface {
Size() int
}
one reason this might be useful, is that for functionality depending on this, you may need to do more than just have a single size function. There's probably instances where you have an average case, worst case etc. There might be some need to extend it.
and later, in mutable state and elsewhere:
type MutableState interface {
AddTransferTasks(transferTasks ...persistence.Task)
AddTimerTasks(timerTasks ...persistence.Task)
GetTransferTasks() []persistence.Task
GetTimerTasks() []persistence.Task
DeleteTransferTasks()
DeleteTimerTasks()
SetUpdateCondition(int64)
GetUpdateCondition() int64
+ size.EstSize // any methods on this interface will be also required by mutable state
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, in this case, we have a Sizeable interface in cache.go which I think will work well here. Or do I need a interface that is separated from the cache?
What changed?
Implement Size() measuring size estimation for execution cache, add empty size estimation function for mutableState and shardContext
Why?
We want to modernize existing cadence common cache implement to a bytes-based system. That means we need to have a method to measure each entry (which is currently accepting any generic interface). We found the "Reflect" package provides a measuring function but runtime is too slow to be used in cache operations. Therefore, we will require all usages to implement the Size() function in their cache logic if they want to migrate to the new bytes-based system.
In order to seamless transition from the current cache system with an entry-based model, the implementation and rollout will be done in following phases:
a. Implement Size() for ExecutionCache <-- This PR
b. Implement Size() for EventCache
How did you test it?
Unit tests
Potential risks
No risk since this PR only adds read-only function that is not used.
Release notes
Documentation Changes