-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
remove zookeeper config requirement #5375
remove zookeeper config requirement #5375
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5375 +/- ##
==========================================
- Coverage 66.46% 4.53% -61.94%
==========================================
Files 240 240
Lines 14569 14567 -2
Branches 642 647 +5
==========================================
- Hits 9684 660 -9024
- Misses 4885 13907 +9022
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -91,7 +91,6 @@ object Invoker { | |||
Map(servicePort -> 8080.toString) ++ | |||
ExecManifest.requiredProperties ++ | |||
kafkaHosts ++ | |||
zookeeperHosts ++ |
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.
Without this configuration, a k8s deployment can still take advantage of the zookeeper?
I think we need to handle the zookeeper dependency in the k8s deployment first.
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.
Yes this just makes it such that you can start the invoker without having the config set. If you try to do that right now it will fail, but this doesn't remove the config from existing just the requirement. Just that you don't need it to start.
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.
If you don't have it set but need it based on the arguments you passed to the invoker, it will hit this same case where the invoker errors out and stops:
// --uniqueName is defined with a valid value, id is empty, assign an id via zookeeper case CmdLineArgs(Some(unique), None, _, overwriteId) => if (config.zookeeperHosts.startsWith(":") || config.zookeeperHosts.endsWith(":")) { abort(s"Must provide valid zookeeper host and port to use dynamicId assignment (${config.zookeeperHosts})") }
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.
ok, I got the intention, this is just to remove the zookeeper hosts from the required configs.
It does not block adding zookeeper hosts if required.
Description
First step to getting rid of the zookeeper dependency on the invokers. Invokers currently can already be started without the zookeeper config if
--id
flag is set which is already the case in ansible. The only case that needs to be covered still before removing the dependency is adding support for kubernetes deployment. If started without config and need to connect to zookeeper still, the invoker will fail and shut down anyways from failing to connect to zookeeper in the instance id assigner on startup so this shouldn't be a breaking change.Related issue and scope
My changes affect the following components
Types of changes
Checklist: