Skip to content

SAMZA-2431: Fix the checkpoint and changelog topic auto-creation. #1251

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.

Sign up for GitHub

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

Jump to bottom
Merged
shanthoosh merged 3 commits into from
Jan 15, 2020
Merged

SAMZA-2431: Fix the checkpoint and changelog topic auto-creation. #1251

shanthoosh merged 3 commits into from
Jan 15, 2020

Conversation

@shanthoosh
Copy link
Contributor

@shanthoosh shanthoosh commented Jan 14, 2020 •
edited by prateekm
Loading

Symptom: Checkpoint and changelog kafka topics of a samza job may be created with cleanup.policy set to 'delete' instead of 'compact' for certain cases.

Cause:

  • Checkpoint: The control-flow in KafkaStreamSpec to build checkpoint spec swallows the essential kafka-topic configuration rather it passes empty configuration bag to kafka-broker. This behavior was introduced in SAMZA-2339.
  • Changelog: The change-log topic configurations were incorrectly generated when the RocksDB store TTL is set to -1 by the user. This behavior was introduced in SAMZA-1929.

Changes: Fix the topic-creation control-flow for the metadata topics and generate the correct topic-configurations.

Tests: Added unit tests to validate that the expected topic configuration bag was generated for both checkpoint and changelog topics.

API Changes: None

Upgrade Instructions: None

Usage Instructions: None

@shanthoosh
Copy link
Contributor Author

shanthoosh commented Jan 14, 2020

@prateekm Can you please take a look at this patch.

prateekm
prateekm requested changes Jan 14, 2020
View reviewed changes
shanthoosh
shanthoosh commented Jan 14, 2020
View reviewed changes
Copy link
Contributor Author

@shanthoosh shanthoosh left a comment

Choose a reason for hiding this comment

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

Thanks for the review.

prateekm
prateekm approved these changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@prateekm prateekm left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

kw2542
kw2542 approved these changes Jan 15, 2020
View reviewed changes
samza-kafka/src/main/java/org/apache/samza/system/kafka/KafkaSystemAdmin.java
checkpointTopicProperties.putAll(spec.getConfig());
kafkaSpec = KafkaStreamSpec.fromSpec(StreamSpec.createCheckpointStreamSpec(spec.getPhysicalName(), spec.getSystemName()))
.copyWithReplicationFactor(Integer.parseInt(new KafkaConfig(config).getCheckpointReplicationFactor().get()))
.copyWithProperties(checkpointTopicProperties);
Copy link
Contributor

@kw2542 kw2542 Jan 15, 2020

Choose a reason for hiding this comment

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

We can do it separately, can we add .copyWithConfig or .copyWithMap which takes Config or Map<String, String> ?

copyWithProperties is not user friendly, as we are converting a Map/Config to a properties, and this method basically convert it back.

Copy link
Contributor Author

@shanthoosh shanthoosh Jan 15, 2020

Choose a reason for hiding this comment

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

Yes, adding the copyWithConfig to KafkaStreamSpec might be good convenience API. We've additional cleanup planned with this control-flow in the near-future. If acceptable, then I can couple this change with that. Since it's a critical bug-fix, I would prefer to keep the change minimal by containing it to the fix alone and do the clean-up later.

@shanthoosh shanthoosh merged commit 80c799d into apache:master Jan 15, 2020
rmatharu-zz pushed a commit to rmatharu-zz/samza that referenced this pull request Jan 21, 2020
@shanthoosh @rmatharu
SAMZA-2431: Fix the checkpoint and changelog topic auto-creation. (ap…
a35de71 
…ache#1251)

* Fix the checkpoint and changelog topic creation configurations.

* Address review comments.

* Address review comments.
lhaiesp pushed a commit that referenced this pull request Feb 6, 2020
@shanthoosh @lhaiesp
SAMZA-2431: Fix the checkpoint and changelog topic auto-creation. (#1251
e250678 
)

* Fix the checkpoint and changelog topic creation configurations.

* Address review comments.

* Address review comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@prateekm prateekm prateekm approved these changes

@kw2542 kw2542 kw2542 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shanthoosh @prateekm @kw2542