-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 ServerView from RealtimeIndexTasks and use coordinator http endpoint for handoff information #2015
Conversation
5571aa2
to
209011f
Compare
@@ -35,6 +35,8 @@ | |||
*/ | |||
public Iterable<TimelineObjectHolder<VersionType, ObjectType>> lookup(Interval interval); | |||
|
|||
public Iterable<TimelineObjectHolder<VersionType, ObjectType>> lookup(Interval interval, boolean incompleteOk); |
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.
allowIncomplete
would be clearer I think. Maybe add some javadoc as well while we're at it.
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.
Can we instead expose the method
public Iterable<TimelineObjectHolder<VersionType, ObjectType>> lookupWithIncomplete(Interval interval);
Boolean flag style method often result in pretty confusing function signatures, because you are not always sure of what the boolean is supposed to mean. Especially if you don't actually have the code. If we can instead move those semantics into a completely different method name, it becomes easier for users to know what is going on.
Also, 👍 on adding javadoc.
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.
fixed.
42641f5
to
f1281c0
Compare
f1281c0
to
c26d333
Compare
👍 looks good to me. If we change the way coordinator discovery is done, we should also change the way overlord discovery in |
|
||
|Property|Description|Default| | ||
|--------|-----------|-------| | ||
|`druid.selectors.coordinator.serviceName`|The druid.service name of the coordinator node. To start the Coordinator with a different name, set it with this property. |druid/coordinator| |
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.
Even if this is "druid/coordinator" by default, it should be set to "coordinator" in the example common.runtime.properties, since the example coordinator properties set the druid.service to "coordinator". Many people start off their clusters by copying the example configs.
I wonder if we should also just change the defaults in the code to match those…
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.
I think the configs were created before we had defaults. I'd rather just change the example configs to match the defaults, or leave it out of the example configs entirely, since it's not necessary.
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 default overlord service name in tranquility is "overlord" to match the examples, so if we do change the examples we should change that too. All three of those things should match though (druid defaults, tranquility defaults, druid example configs)
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.
#2046 to track
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.
bouncing for travis. |
Also added graph showing the improvement on zookeeper load with ~500 realtime index tasks running in PR description. |
Remove ServerView from RealtimeIndexTasks and use coordinator http endpoint for handoff information
I think this patch makes the rolling update process not work, since it suggests doing coordinators after indexing nodes, but we'd need the coordinator endpoint up first. |
added a "release notes" label |
We also need to add to the release notes that you need to make sure your |
@gianm oddly this PR is not even mentioned at all in the release notes |
Probably it was added after the first RC and missed. We should double check that all the stuff after that actually made it into the notes. |
@xvrl will take a look now |
@xvrl updated the release notes & added a few other missing things |
@gianm thx I was updating the release notes but you beat me to it. |
I just realized that the new endpoint isn't documented at http://druid.io/docs/latest/design/coordinator.html. Should it be? Or, are there some types of endpoints that are deliberately not documented at druid.io/docs and this is one of them? |
@rasahner This should be documented. Can you submit a PR? |
see #2238 |
@nishantmonu51 @gianm @pjain1 @xvrl @cheddar None of the examples were updated to reflect that serviceName now talks to coordinator and realtimes won't be able to do handoff. |
Also, does anything that requires the coordinator to talk to the overlord still work? Kill tasks, merge tasks, etc? Edit: yes they will, I thought we'd removed indexing.serviceName for a second there |
which is responsible for tracking segment handoffs and doing callbacks
when handoff is complete.
coordinator to get serverView
Below image shows the improvements by above changes on zookeeper read load with nearly 500 realtime index tasks running -