-
Notifications
You must be signed in to change notification settings - Fork 334
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
added new config for job.changelog.system #31
Conversation
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.
Changes look good. I do suggest adding documentation to the website regarding the new config.
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.
+1 on @navina's comment on the documentation. LGTM, just one suggestion on an error case.
&& changelogSystem.isDefined) { | ||
// get the system name | ||
Some(changelogSystem.get + "." + systemStream) | ||
} else { |
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.
Shouldn't it error out if systemStream only has stream name and changelogSystem is not defined?
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 was current behavior. I will add this check.
def testIsChangelogSystemSetting { | ||
val configMap = Map[String, String]( | ||
FACTORY.format("store1") -> "some.factory.Class", | ||
CHANGELOG_STREAM.format("store1") -> "system1.stream1", |
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.
Also, with this change, I think that we will need to update our internal config rewriter as well, to detect all systems used by the job.
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.
Good point, but I think this change is not required in the open source.
// these values will be combined into <asystem>.<astream> | ||
val systemStream = getOption(CHANGELOG_STREAM format name) | ||
val changelogSystem = getOption(CHANGELOG_SYSTEM) | ||
val systemStreamRes = if ( systemStream.isDefined |
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.
May be you can simplify this if-else statement as :
if (systemStream.isDefined && !systemStream.getOrElse("").contains(".")) {
if (changelogSystem.isDefined) {
Some(changelogSystem.get + "." + systemStream.get)
} else {
throw new SamzaException(...)
}
} else {
...
}
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.
1 comment. Otherwise, looks good!
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.
Looks good. +1
As per subject, it's a demo of stream-to-table join using RocksDb. Author: Wei Song <wsong@linkedin.com> Reviewers: Prateek Maheshwari <pmaheshwari@apache.org> Closes apache#31 from weisong44/latest
* SAMZA-2797: Call flush during stop from CoordinatorStreamWriter (apache#1692) * SAMZA-2798: Populate worker.opts in environment variable only if available (apache#1693) Description Populate worker.opts in the environment variable only if available in the configs. Changes Check if worker.opts is present and then add it to environment variable Tests Updated unit tests * Add MAX_BACKGROUND_JOBS config for RocksDB (apache#1694) * SAMZA-2784: Remove excessive commit logs (apache#1695) * SAMZA-2799: Remove worker.opts handling in shell command builder (apache#1696) --------- Co-authored-by: ajo thomas <ajo.thomas24@gmail.com> Co-authored-by: Bharath Kumarasubramanian <bharathkk@apache.org> Co-authored-by: Shekhar Sharma <72765053+shekhars-li@users.noreply.github.com> Co-authored-by: Daniel Chen <xrchen@uwaterloo.ca>
SAMZA-1060.
Allow to specify a changelog system separately, so user can only specify stream name for each store.
If user specifies both (system and stream) it overwrites the job.changelog.system setting.