Skip to content

SAMZA-2441: Update ApplicationRunnerMain#ApplicationRunnerCommandLine not to load local file#1265

Merged
lhaiesp merged 2 commits into
apache:masterfrom
kw2542:SAMZA-2441
Feb 4, 2020
Merged

SAMZA-2441: Update ApplicationRunnerMain#ApplicationRunnerCommandLine not to load local file#1265
lhaiesp merged 2 commits into
apache:masterfrom
kw2542:SAMZA-2441

Conversation

@kw2542
Copy link
Copy Markdown
Contributor

@kw2542 kw2542 commented Jan 31, 2020

This is backward incompatible and is only supposed to be included in Samza 1.5

Design:
https://cwiki.apache.org/confluence/display/SAMZA/SEP-23%3A+Simplify+Job+Runner

Changes:

  1. Override ApplicationRunnerCommandLine#loadConfig not to invoke config loader as ApplicationRunners handles initial config and full job config themselves.
  2. Add JobRunnerCommandLine to load full job config for JobRunner to keep consistent with previous behavior as it still needs full job config for special use cases.

API Changes:
None

Upgrade Instructions:
None

Usage Instructions:
None

Tests:

  1. Unit Tests

… not to load local file

Design:
https://cwiki.apache.org/confluence/display/SAMZA/SEP-23%3A+Simplify+Job+Runner

Changes:
1. Override ApplicationRunnerCommandLine#loadConfig not to invoke config loader.
2. Add JobRunnerCommandLine for JobRunner as it still needs full job config for special use cases.

API Changes:
None

Upgrade Instructions:
None

Usage Instructions:
None

Tests:
1. Unit Tests
}

@Override
public Config loadConfig(OptionSet options) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you feel that the "base" use case for CommandLine is to need the full config? To me, it looks a little awkward to need to override loadConfig here with an implementation which is almost the same as the base loadConfig.
I suppose one alternative would be to make this impl here the base impl, and then have each use case call ConfigUtil.loadConfig if is needs it. What do you see as the tradeoffs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question, in all 13 direct usage of CommandLine, and 5 sub classes of CommandLine, they are all expecting full job configs to work properly, e.g. RocksDbReadingTool extends CommandLine to read full job config before being able to read data from Rocks DB, which is being invoked by CLI.

If we change CommandLine to the other way, that means we need to invoke ConfigUtil.loadConfig explicitly in 13 places, and override loadConfigs in 5 sub classes to invoke ConfigUtil.loadConfig too, compared to overriding once in ApplicationRunnerCommandLine only.

I would prefer the current way to keep the base class to serve majority use case.

Comment thread samza-core/src/test/java/org/apache/samza/runtime/TestApplicationRunnerMain.java Outdated
Copy link
Copy Markdown
Contributor

@lhaiesp lhaiesp left a comment

Choose a reason for hiding this comment

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

LGTM

@lhaiesp lhaiesp merged commit 5a58877 into apache:master Feb 4, 2020
@kw2542 kw2542 deleted the SAMZA-2441 branch February 4, 2020 23:32
cameronlee314 added a commit to cameronlee314/samza that referenced this pull request Feb 7, 2020
…mandLine not to load local file (apache#1265)"

This reverts commit 5a58877.

Reverting this change for the 1.4.0 branch since it is backwards
incompatible and we don't want to release this for 1.4.
cameronlee314 added a commit that referenced this pull request Feb 7, 2020
…mandLine not to load local file (#1265)" (#1268)

This reverts commit 5a58877.

Reverting this change for the 1.4.0 branch since it is backwards
incompatible and we don't want to release this for 1.4.
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