Merge core CoordinatorClient with MSQ CoordinatorServiceClient.#14652
Merge core CoordinatorClient with MSQ CoordinatorServiceClient.#14652gianm merged 6 commits intoapache:masterfrom
Conversation
Continuing the work from apache#12696, this patch merges the MSQ CoordinatorServiceClient into the core CoordinatorClient, yielding a single interface that serves both needs and is based on the ServiceClient RPC system rather than DruidLeaderClient. Also removes the backwards-compatibility code for the handoff API in CoordinatorBasedSegmentHandoffNotifier, because the new API was added in 0.14.0. That's long enough ago that we don't need backwards compatibility for rolling updates.
kfaraz
left a comment
kfaraz
left a comment
There was a problem hiding this comment.
Looks good, minor queries.
| } | ||
| if (!handOffCallbacks.isEmpty()) { | ||
| log.warn("Still waiting for Handoff for [%d] Segments", handOffCallbacks.size()); | ||
| log.info("Still waiting for handoff for [%d] segments", handOffCallbacks.size()); |
There was a problem hiding this comment.
Nit: "handoff of x segments"?
There was a problem hiding this comment.
Should we raise an alert if handoff wait time exceeds a threshold?
There was a problem hiding this comment.
Nit: "handoff of x segments"?
Sure, that sounds nicer to me as well. I will change it if there is some other reason to make changes (like if the CI fails).
Should we raise an alert if handoff wait time exceeds a threshold?
There's already an alert raised if the handoffConditionTimeout is exceeded, which post #14539 would default to 15 mins.
| Collections.singletonList(interval) | ||
| usedSegments = FutureUtils.getUnchecked( | ||
| coordinatorClient.fetchUsedSegments(dataSource, Collections.singletonList(interval)), | ||
| true |
There was a problem hiding this comment.
I wonder if we still need the retry logic in the catch block below. It would be handled by the new CoordinatorClient itself.
There was a problem hiding this comment.
Good point, I'll remove this part.
| * Retry policy that uses up to about an hour of total wait time. Note that this is just the total waiting time | ||
| * between attempts. It does not include the time that each attempt takes to execute. | ||
| */ | ||
| public static StandardRetryPolicy aboutAnHour() |
There was a problem hiding this comment.
Nit: Would StandardRetryPolicy.retryUptoAnHour() or StandardRetryPolicy.retryForAnHour() communicate the intent better?
The "about" part could be left out as the approximate nature of the backoffs is probably a given. Although, I don't feel strongly about it.
There was a problem hiding this comment.
Hmm, to me they seem similar, so I'm inclined to merge the patch rather than have CI run again 🙂
Saving the planet by doing fewer CI runs!
3 participants
Continuing the work from #12696, this patch merges the MSQ CoordinatorServiceClient into the core CoordinatorClient, yielding a single interface that serves both needs and is based on the ServiceClient RPC system rather than DruidLeaderClient.
Release notes: Also removes the backwards-compatibility code for the handoff API in CoordinatorBasedSegmentHandoffNotifier, because the new API was added in 0.14.0. That's long enough ago that we don't need backwards compatibility for rolling updates.