-
Notifications
You must be signed in to change notification settings - Fork 2
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
[DPE-5079]: Kyuubi HA with kyuubi <> zookeeper integration #26
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.
The code looks good to me! I just have a couple of comments/suggestions, but they are not blocking
@deusebio Thanks a lot for the review! I agree with your points. Since we're going to merge this to the feature/kyuubi-ha branch, I'd rather piggyback these non-blocking suggestions in the next PR I open in the same feature (that way, the CI need not be triggered again). At the end, these will appear in I'm waiting for @welpaolo's review here and once I have that, I'll merge this and push your suggestions in the next PR in the same feature/epic. |
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.
LGTM, I left a couple of minor comments
I'm merging this now to |
Please note that the base branch is
feature/kyuubi-ha
. This is because I wanted to have full HA working in thefeature/kyuubi-ha
branch before we merge it tomain
and release it. All other subsequent works on Kyuubi HA will be merged tofeature/kyuubi-ha
and at the end, this feature branch will be merged tozookeeper-integration
.