Skip to content

[FLINK-25235][tests] re-enable zookeeper test #18949

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
XComp merged 1 commit into from
Mar 14, 2022
Merged

[FLINK-25235][tests] re-enable zookeeper test #18949

XComp merged 1 commit into from
Mar 14, 2022

Conversation

@niklassemmler
Copy link

@niklassemmler niklassemmler commented Mar 1, 2022

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 1, 2022 •
edited
Loading

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@niklassemmler niklassemmler force-pushed the FLINK-25235-reenable-zookeeper-test branch 3 times, most recently from b1cd905 to b6c65b2 Compare March 9, 2022 15:55
autophagy
autophagy reviewed Mar 10, 2022
View reviewed changes
@niklassemmler niklassemmler force-pushed the FLINK-25235-reenable-zookeeper-test branch from b6c65b2 to 793d98f Compare March 11, 2022 10:06
autophagy
autophagy approved these changes Mar 11, 2022
View reviewed changes
Copy link
Contributor

@autophagy autophagy left a comment

Choose a reason for hiding this comment

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

I think this looks good to me - thanks for addressing my comments 🙂

@niklassemmler niklassemmler force-pushed the FLINK-25235-reenable-zookeeper-test branch from 793d98f to 7941056 Compare March 11, 2022 13:44
XComp
XComp approved these changes Mar 13, 2022
View reviewed changes
Copy link
Contributor

@XComp XComp left a comment

Choose a reason for hiding this comment

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

Thanks @metaswirl and @autophagy for the contribution. I did another pass over it. The changes look sufficient now as far as I can see. I did some slight adjustments to the code structure and added more comments to describe the special setup of the TestingMiniCluster with multiple JobManager. PTAL, @metaswirl and feel free to squash and rebase the branch if you agree

@XComp XComp force-pushed the FLINK-25235-reenable-zookeeper-test branch from d8094ba to 0024e51 Compare March 13, 2022 15:15
@XComp
Copy link
Contributor

XComp commented Mar 13, 2022

I squashed and rebased the branch after an offline 👍 from @metaswirl

[FLINK-25235][runtime] Re-enable ZooKeeper test with multi-component …
f42ce82 
…leader election (see FLINK-24038)

The TestingMiniCluster had to be adapted a bit to make this work.
The new multi-component leader election assumes that there's a
single leader election instance available per JobManager that is
closed as soon as the JobManager is shut down. The TestingMiniCluster
used a single leader election that's shared between multiple
JobManagers and closed after all these JMs are shutdown.

The new implementation creates an individual
HighAvailabilityServices instance per JM and closes this instance as
soon as the JM shuts down to revoke the leadership and enable other
JMs to pick the leadership up again.
@niklassemmler niklassemmler force-pushed the FLINK-25235-reenable-zookeeper-test branch from 0024e51 to f42ce82 Compare March 14, 2022 08:15
@XComp
Copy link
Contributor

XComp commented Mar 14, 2022

I created a follow-up ticket FLINK-26630 that covers the missing sharing of certain HA components like the JobGraphStore and the JobResultStore and also addresses why it's ok to have it as a follow-up rather than fixing it as part of this PR.

@XComp XComp merged commit c01e537 into apache:master Mar 14, 2022
@XComp
Copy link
Contributor

XComp commented Mar 14, 2022

The CI run was success and I verified that ZooKeeperLeaderElectionITCase ran as part of that build. I merged the PR. Thanks again for the contribution

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

Reviewers

@XComp XComp XComp approved these changes

+1 more reviewer

@autophagy autophagy autophagy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component=Runtime/Coordination

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@niklassemmler @flinkbot @XComp @autophagy @rmetzger