-
Notifications
You must be signed in to change notification settings - Fork 118
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 MetadataService
to Use ContentTransformer
#1069
Conversation
Motivation: Previously, the Central Dogma server in `MetadataService` would repeatedly attempt to commit until it succeeded. With the introduction of `ContentTransformer` in line#1064, the server can now handle transformations in a single attempt, avoiding unnecessary retries. Modifications: - Updated `MetadataService` to utilize `ContentTransformer` for commit operations. Result: - Improved efficiency in `MetadataService` by eliminating repeated commit attempts.
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.
Checked that:
- there are no significant differences between previous/migrated logic
- there is no additional IO/blocking calls within
ContentTransformer
I'm unsure about the scope since only some methods in MetadataService
seem to be migrated while others haven't been migrated, but the changes look good to me
final Tokens tokens = tokens(node); | ||
final Token token = tokens.get(appId); // Raise an exception if not found. | ||
if (token.deactivation() == null) { | ||
throw new IllegalArgumentException("The token is already activated: " + appId); |
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.
Note: this type of logic may be less performant in practice since a lock is always held (whereas previously this was done with an optimistic-lock).
Having said this, I agree with this approach since this API is not used often and I prefer code clarity over minor optimizations.
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.
That's something I overlooked. 😅
If an exception is raised, the ZooKeeper lock won't be held at all.
As you mentioned, it won't be an issue in most cases. If it becomes a problem, I can update the implementation to read the content first.
That is correct. 👍
The rest of them are migrated in #1061 |
Motivation:
Previously, the Central Dogma server in
MetadataService
would repeatedly attempt to commit whenChangeConflictException
is raised.With the introduction of
ContentTransformer
in Add Transforming Content Push Command #1064, the server can now handle transformations in a single attempt, avoiding unnecessary retries.Modifications:
MetadataService
to utilizeContentTransformer
for commit operations.Result:
MetadataService
by eliminating repeated commit attempts.To-do:
tokenRepo
andmetadataRepo
inMetadataService
into its class respectively.