SAMZA-2171 : Implement MetadataResourceManager to encapsulate loading of metadata resources #1006
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnishimura
commented
Apr 23, 2019
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceLoader.java
Outdated
Show resolved
Hide resolved
dnishimura
commented
Apr 24, 2019
| * Creates and loads the required metadata resources for checkpoints, changelog stream and other | ||
| * resources related to the metadata system | ||
| */ | ||
| public void createResources() { |
There was a problem hiding this comment.
For the follow on startpoint work, I plan to add either the fan out call here for the fan out model or the ack clean up call here for the intent-ack model.
dnishimura
changed the title
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/zk/ZkJobCoordinator.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
prateekm
reviewed
Apr 26, 2019
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
dnishimura
commented
Apr 26, 2019
There was a problem hiding this comment.
Thanks @cameronlee314 and @prateekm for your reviews. Please see my responses.
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceManager.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/zk/ZkJobCoordinator.java
Outdated
Show resolved
Hide resolved
dnishimura
commented
Apr 29, 2019
| * Creates and loads the required metadata resources for checkpoints, changelog stream and other | ||
| * resources related to the metadata system | ||
| */ | ||
| public void createResources() { |
There was a problem hiding this comment.
For the follow on startpoint work, I plan to add either the fan out call here for the fan out model or the ack clean up call here for the intent-ack model.
|
@cameronlee314 & @prateekm please see my responses and updates. Thanks. |
cameronlee314
approved these changes
Apr 29, 2019
samza-core/src/test/java/org/apache/samza/coordinator/TestMetadataResourceUtil.java
Outdated
Show resolved
Hide resolved
shanthoosh
reviewed
Apr 29, 2019
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceUtil.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceUtil.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala
Outdated
Show resolved
Hide resolved
|
@prateekm do you have further comments/concerns? Otherwise, will merge. |
@cameronlee314 - He told me offline he wasn't actively reviewing this PR and he gave a 👍 for my response on his sole comment. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation: Before this PR, the code to create the checkpoint and changelog streams are copy and pasted to each JobCoordinator implementation. This PR encapsulates that logic in a
MetadataResourceManagerso that future JobCoordinator implementations don't have to copy and paste duplicate logic and we can add other metadata resources that we may want to create in the future.@shanthoosh @prateekm @cameronlee314 - please take a look when you get a chance. The likely follow on is to add the startpoint manager fan out call in the
MetadataResourceManager#createResourcesmethod.