Skip to content
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

adding the forceSync flag for handling conflicting topic configs #170

Closed
wants to merge 6 commits into from
Closed

adding the forceSync flag for handling conflicting topic configs #170

wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 26, 2019 •
edited by neoword
Loading

stream-registry PR

Adding flag to provide an option to override user-provided topic config in case of already existing conflicting configs on the underlying brokers (#114)

Added

  • Boolean flag forceSync which can be set by a user while upserting a stream to override the provided topic configs in the stream and continue using the existing topic configs on the brokers.

PR Checklist Forms

  • CHANGELOG.md updated
  • Reviewer assigned
  • PR assigned (presumably to submitter)
  • Labels added (enhancement, bug, documentation)

OneCricketeer
OneCricketeer suggested changes Apr 26, 2019
View reviewed changes
...rc/main/java/com/homeaway/streamplatform/streamregistry/provider/impl/KafkaInfraManager.java
if (forceSync) {
// NOTHING TO DO!
log.info("topic config mismatch for {} ignored. Input config={}, actual (retained) config={}", topic, topicConfigMap, actualTopicConfig);
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly is being "forced" if all you do is continue?

Copy link
Author

Choose a reason for hiding this comment

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

Well, the flag is "forcing" the stream-registry to sync the given topic configs with what exists already on the underlying brokers.
I guess I can say that by continueing here, the code is "forcing" the flow to not go ahead to the following exception generation ..

Copy link
Contributor

Choose a reason for hiding this comment

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

I put alternative names in #114

Copy link
Contributor

@OneCricketeer OneCricketeer Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

ignoring any user provided info, and only updating SR with the underlying settings

The way I interpret that is to take actualTopicConfig and overwrite topicConfigMap with it... i.e. "forcing" the actual configs to "sync" to the Stream Config

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't continue actually perform the UPDATE???
I agree with @Cricket007 remarks on actualTopicConfig and topicConfigMap... where we should be overriding what the user provided, with what is actually out there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't continue actually perform the UPDATE?

No, it breaks here and goes to the next iteration of the loop. updateTopic isn't called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. I missed a curly-brace close.

line 270 is INSIDE not... outside the loop.
https://github.com/ExpediaGroup/stream-registry/pull/170/files/09090b33c68b43bf046a36ddd7b7ed2959b849e3#diff-b4dcb632004addc439896ec21e5b7dabR270

@neoword neoword assigned ghost May 7, 2019
@neoword neoword added the enhancement New feature or request label May 7, 2019
@neoword neoword requested a review from arunvasudevan May 7, 2019 16:32
@neoword neoword added this to the 0.5.1 milestone May 7, 2019
@neoword
Copy link
Contributor

neoword commented May 7, 2019

Some clarification on forceSync=true:

  • This should SUCCEED, IF AND ONLY IF config is THE SAME on all underlying clusters
  • This should FAIL, if any of the cluster (stream-bindings) differ, with an error having a call to action to make fix the corresponding clusters that are different.

@neoword neoword requested review from neoword and removed request for arunvasudevan May 7, 2019 17:48
@neoword
Copy link
Contributor

neoword commented May 7, 2019

Waiting for more info from team before I comment on this PR.

neoword
neoword suggested changes May 7, 2019
View reviewed changes
...rc/main/java/com/homeaway/streamplatform/streamregistry/provider/impl/KafkaInfraManager.java
if (forceSync) {
// NOTHING TO DO!
log.info("topic config mismatch for {} ignored. Input config={}, actual (retained) config={}", topic, topicConfigMap, actualTopicConfig);
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't continue actually perform the UPDATE???
I agree with @Cricket007 remarks on actualTopicConfig and topicConfigMap... where we should be overriding what the user provided, with what is actually out there.

@neoword
Copy link
Contributor

neoword commented May 7, 2019

Also should forceSync=true fail if no underlying topic exists?

neoword
neoword suggested changes May 7, 2019
View reviewed changes
...rc/main/java/com/homeaway/streamplatform/streamregistry/provider/impl/KafkaInfraManager.java
if (forceSync) {
// NOTHING TO DO!
log.info("topic config mismatch for {} ignored. Input config={}, actual (retained) config={}", topic, topicConfigMap, actualTopicConfig);
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. I missed a curly-brace close.

line 270 is INSIDE not... outside the loop.
https://github.com/ExpediaGroup/stream-registry/pull/170/files/09090b33c68b43bf046a36ddd7b7ed2959b849e3#diff-b4dcb632004addc439896ec21e5b7dabR270

@arunvasudevan
Copy link
Contributor

Here are my comments on the PR:

  1. forceSync is definitely a mis-guiding name..either ignoreTopicCreationIfConfigsMismatch or skipTopicCreationIfConfigsMismatch or something along that lines.

  2. I can see this PR as a followup to the PR (Stop Stream Creation for existing topics if configs don't match #52).
    If the provided configs are different from the underlying topic config we have 2 options:

    • Force the underlying topic with the provided config -> We don't want to do this based on the above PR (Stop Stream Creation for existing topics if configs don't match #52) and the issue that led to it.
    • Ignore (or skip) topic creation -> This is safer than the above option but the user may not notice the log in the CI/CD and it could lead to unexpected behavior later in the application.

Why not create topic only at the vpc and not in the replicatedVPC? creating topics in the replicatedVPC is a replication tools task isn't it?

@ghost
Copy link
Author

ghost commented May 16, 2019

Since (#178) is merged, this PR is not required.

@ghost ghost closed this May 16, 2019
@ghost ghost deleted the addForceSyncTopic branch August 7, 2019 20:48
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.5.1
Development

Successfully merging this pull request may close these issues.

Consider adding forceSync=true for topic updates
4 participants
@neoword @arunvasudevan @OneCricketeer @krishnatippur