Optional segment load/drop management without zookeeper using http by himanshug · Pull Request #4874 · apache/druid

himanshug · 2017-09-28T15:58:46Z

~~TODOs:~~

~~Test on a cluster with decent load.~~

This patch introduces following changes.

On Historical Side:

All non-zookeeper related code is extracted out of ZkCoordinator and moved into new class SegmentLoadDropHandler. ZkCoordinator now uses SegmentLoadDropHandler to delegate load/drop requests.
A new method ListenableFuture SegmentLoadDropHandler.processBatch(List<DataSegmentChangeRequest>) is added to support http endpoint for segment load/drop management.
New endpoint POST /druid-internal/v1/segments/changeRequests is added in SegmentListerResource that uses ListenableFuture SegmentLoadDropHandler.processBatch(List<DataSegmentChangeRequest>) to delegate batch of load/drop requests received. Here is a copy-paste of javadoc for this endpoint.

  /**
   * This endpoint is used by HttpLoadQueuePeon to assign segment load/drop requests batch. This endpoint makes the
   * client wait till one of the following events occur. Note that this is implemented using async IO so no jetty
   * threads are held while in wait.
   *
   * (1) Given timeout elapses.
   * (2) Some load/drop request completed.
   *
   * It returns a map of "load/drop request -> SUCCESS/FAILED/PENDING status" for each request in the batch.
   */

druid.segmentCache.numLoadingThreads configuration is revived and must be greater than the "druid.coordinator.loadqueuepeon.http.batchSize described later.

On Coordinator Side:

CuratorLoadQueuePeon is created by copying existing LoadQueuePeon class.
LoadQueuePeon is made abstract class.
HttpLoadQueuePeon is created to do segment load/drop management via http. It uses the new endpoint introduced above to send batch of load/drop requests to Historical node.
Following new configurations are introduced.
druid.coordinator.loadqueuepeon.type = http or curator, curator by default
druid.coordinator.loadqueuepeon.http.batchSize = number of load/drop to try and process in parallel, must be less than druid.segmentCache.numLoadingThreads on historical
druid.coordinator.loadqueuepeon.http.repeatDelay = delay Duration for periodic check for new load/drop request to send to historical. note this could be very large given that same code is executed whenever a new load/drop is requested to HttpLoadQueuePeon without waiting for schedule to kick in, default 1 minute
druid.coordinator.loadqueuepeon.http.hostTimeout = default 5 minutes, timeout used to be specified on new endpoint introduced for batch load/drop request.

In the long run, I plan to remove CuratorLoadQueuePeon and ZkCoordinator. They are being kept now only to be backward compatible and for HttpLoadQueuePeon to prove itself in production.

Documentation for HttpLoadQueuePeon is intentionally left out.

cheddar · 2017-10-04T18:16:47Z

Pair is a really bad class to build an API around. It is poorly defined in terms of what the LHS and RHS are. Please remove the JSON annotations here and instead create a concrete class that actually represents what you are sending across the wire.

In general, if you ever see Pair<> on any sort of public function signature or API, it was done incorrectly.

not using Pair anymore.

cheddar · 2017-10-04T18:18:30Z

You did not document this, was that intentional?

yes, will let it run on our servers for a while before documenting. hopefully we'll have enough confidence in this before 0.11.1 release so will get documented before that release.

cheddar · 2017-10-04T18:21:47Z

This annotation is an abomination. Just make it public

This constructor requires creator to pass the ExecutorService which is very handy for tests to be able to deterministic. However, I don't really want any other code to use this constructor and hence added that annotation. If you still believe that its not useful then I'll remove it.

cheddar · 2017-10-04T18:23:50Z

Are there no extra objects you could log out with this to provide more information in the log?

no, just wanted to mark start/stop in log to check whether it indeed gets started/stopped etc.

cheddar · 2017-10-04T18:27:04Z

Is this doing a cache of the load/drop requests or a cache of the segments that have already been stored? If the latter, Niketh has a PR that moves the cache to be co-located with the segment storage instead of a separate cache directory. This won't follow that logic, please explore.

unrelated to work in this PR. its a copy paste from ZkCoordinator and this PR is keeping old things exactly same.

cheddar · 2017-10-05T16:00:08Z

This is acting like these requests succeeded, is that the correct thing to do?

yes, this behavior is retained from existing behavior in CuratorLoadQueuePeon (copy of older LoadQueuePeon class )

cheddar · 2017-10-05T16:02:55Z

What's the difference between this and dropSegment?

this behavior is retained from existing behavior in CuratorLoadQueuePeon (copy of older LoadQueuePeon class ). I am not sure about this but it was recently added so kept.

cheddar · 2017-10-05T16:03:45Z

Should this really be returning the set, which is likely mutable?

this behavior is retained from existing behavior in CuratorLoadQueuePeon (copy of older LoadQueuePeon class ). but changed to return unmodifiable version.

cheddar · 2017-10-05T16:06:55Z

What's the point of this type thing? We should either leverage polymorphism or completely eliminate it. Using it sometimes and not other times gets confusing

changed to use Polymorphism.

cheddar · 2017-10-05T16:47:05Z

I really don't understand this cancellation future stuff....

this code is removed.

This reverts commit e725ff4.

)" This reverts commit d8b3bfa.

himanshug force-pushed the http_segment_assignment branch 2 times, most recently from 50043d1 to 103d434 Compare October 3, 2017 16:57

cheddar requested changes Oct 5, 2017

View reviewed changes

himanshug changed the title ~~[WIP] Optional segment load/drop management without zookeeper using http~~ Optional segment load/drop management without zookeeper using http Oct 5, 2017

himanshug added 8 commits October 11, 2017 12:06

Revert "1-based counts in ZkCoordinator (apache#4917)"

59b6c5d

This reverts commit e725ff4.

Revert "Remove SegmentLoaderConfig.numLoadingThreads config (apache#4829

edb9bfa

)" This reverts commit d8b3bfa.

introducing CuratorLoadQueuePeon

6631ead

HttpLoadQueuePeon based off of current code

edcd0a2

SegmentLoadDropHandler copy/pasted from ZkCoordinator

92f8851

remove non-zk part from ZkCoordinator

eb37b91

remove zk part from SegmentLoadDropHandler

8a7f0a4

additional changes for segment load/drop management with http

11d21ed

himanshug force-pushed the http_segment_assignment branch 2 times, most recently from ec0b74f to 20ca85b Compare October 11, 2017 20:31

address review comments

c8d1c0a

himanshug force-pushed the http_segment_assignment branch from 20ca85b to c8d1c0a Compare October 11, 2017 21:22

himanshug closed this Oct 17, 2017

himanshug mentioned this pull request Oct 17, 2017

Optional segment load/drop management without zookeeper using http #4966

Merged

pjain1 mentioned this pull request Oct 21, 2017

Kafka Index Task that supports Incremental handoffs #4815

Merged

This was referenced Feb 2, 2018

HTTP segment load/drop peons getting unauthorized #5325

Closed

CliCoordinator: LoadQueueTaskMaster should use an escalated http client. #5329

Merged

Conversation

himanshug commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

himanshug commented Sep 28, 2017 •

edited

Loading