Use Worker instead of ZkWorker whenever possible by drcrallen · Pull Request #2249 · apache/druid

drcrallen · 2016-01-11T23:57:38Z

~~Depends on #1953~~

drcrallen · 2016-01-13T16:00:58Z

I think this line snuck in during some of my commit shuffling. I'll split it out before final rebase

drcrallen · 2016-01-22T22:20:35Z

Will revert this one. Stale changes from old commit

gianm · 2016-02-09T04:19:40Z

Why does this have to happen? These properties seems like they should work on their own for everything except lastCompletedTaskTime.

Also in light of the need for overriding it'd probably be cleaner to generate the disabledWorker every time someone posts a disable call. That happens so infrequently that there is not really much need to optimize it…

that's reasonable

gianm · 2016-02-09T04:28:36Z

Broadly looks good, I assume this is needed for #2246. would like to understand the property injection change better though.

himanshug · 2016-02-12T19:50:34Z

why is this needed?

drcrallen · 2016-02-16T18:10:14Z

@gianm / @himanshug rebased.

drcrallen · 2016-02-16T21:06:04Z

failedin

Tests run: 7, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.047 sec <<< FAILURE! - in io.druid.indexing.overlord.config.RemoteTaskRunnerConfigTest
testHashCode(io.druid.indexing.overlord.config.RemoteTaskRunnerConfigTest)  Time elapsed: 0.013 sec  <<< FAILURE!
java.lang.AssertionError: Values should be different. Actual: 1709774762
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failEquals(Assert.java:185)
    at org.junit.Assert.assertNotEquals(Assert.java:161)
    at org.junit.Assert.assertNotEquals(Assert.java:198)
    at org.junit.Assert.assertNotEquals(Assert.java:209)
    at io.druid.indexing.overlord.config.RemoteTaskRunnerConfigTest.testHashCode(RemoteTaskRunnerConfigTest.java:335)

investigating

drcrallen · 2016-02-16T21:12:43Z

was error in new code. fixed

drcrallen · 2016-02-16T21:35:42Z

@himanshug / @gianm updated

gianm · 2016-02-19T18:42:11Z

appears unused

drcrallen · 2016-02-19T20:02:29Z

@gianm updated / addressed

gianm · 2016-02-19T20:02:38Z

👍

drcrallen · 2016-02-19T20:27:48Z

test failed from updated unit tests with missing parameters in master. fixed

nishantmonu51 · 2016-02-23T22:54:18Z

  public DateTime getLastCompletedTaskTime()
  {
-    return lastCompletedTaskTime.get();
+    return worker.get().getLastCompletedTaskTime();


can't see the variable lastCompletedTaskTime getting deleted ?

Class field was leftover value that was accidentally used in toString, removed

nishantmonu51 · 2016-02-23T22:57:36Z

👍, only one minor nit about unused variable.

* Moves last run task state information to Worker * Makes WorkerTaskRunner a TaskRunner which has interfaces to help with getting information about a Worker

Use Worker instead of ZkWorker whenever possible

gianm · 2016-03-03T01:17:13Z


-  public Collection<ZkWorker> getWorkers()
+  @Override
+  public Collection<Worker> getWorkers()


@drcrallen this change made the overlord /druid/indexer/v1/workers API no longer return a runningTasks key for each worker, which may confuse some folks' rolling update scripts and definitely makes the web UI less useful.

how do you feel about having the OverlordResource specifically call getZkWorkers if the runner is an RTR? or would it make more sense to add a list of running tasks to the Worker object?

((spoke in chat)) since io.druid.indexing.overlord.http.OverlordResource#getWorkers already special-cases the RTR, suggest making it get zk workers in that case until a more complete solution can be put in place.

Restores old behavior of this api, from before apache#2249 when getWorkers returned ZkWorkers.

drcrallen mentioned this pull request Jan 11, 2016

[WIP] Task tiering #2246

Closed

drcrallen reviewed Jan 13, 2016
View reviewed changes

drcrallen force-pushed the workerExpanded branch from f50ecba to 0439a55 Compare January 22, 2016 21:55

drcrallen reviewed Jan 22, 2016
View reviewed changes

drcrallen added this to the 0.9.1 milestone Feb 9, 2016

drcrallen force-pushed the workerExpanded branch from d2d6620 to 2a45ac4 Compare February 9, 2016 03:58

gianm reviewed Feb 9, 2016
View reviewed changes

himanshug reviewed Feb 12, 2016
View reviewed changes

drcrallen force-pushed the workerExpanded branch 2 times, most recently from 8cf88d8 to 7cd7ab2 Compare February 16, 2016 18:09

drcrallen force-pushed the workerExpanded branch from 7cd7ab2 to 4c00347 Compare February 16, 2016 21:12

drcrallen force-pushed the workerExpanded branch from 4c00347 to d5aa7ff Compare February 16, 2016 21:33

gianm reviewed Feb 19, 2016
View reviewed changes

Comment thread common/src/main/java/io/druid/concurrent/Execs.java Outdated

Copy link
Copy Markdown

Contributor

gianm Feb 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appears unused

drcrallen force-pushed the workerExpanded branch from d5aa7ff to 3944fe9 Compare February 19, 2016 20:02

drcrallen force-pushed the workerExpanded branch from 3944fe9 to 8f63b4a Compare February 19, 2016 20:27

drcrallen mentioned this pull request Feb 22, 2016

Add pending task based resource management autoscaling strategy #2086

Merged

nishantmonu51 reviewed Feb 23, 2016
View reviewed changes

Use Worker instead of ZkWorker whenver possible

ac13a59

* Moves last run task state information to Worker * Makes WorkerTaskRunner a TaskRunner which has interfaces to help with getting information about a Worker

drcrallen force-pushed the workerExpanded branch from 8f63b4a to ac13a59 Compare February 23, 2016 23:02

gianm added the Improvement label Feb 24, 2016

nishantmonu51 added a commit that referenced this pull request Feb 24, 2016

Merge pull request #2249 from metamx/workerExpanded

fb7eae3

Use Worker instead of ZkWorker whenever possible

nishantmonu51 merged commit fb7eae3 into apache:master Feb 24, 2016

drcrallen deleted the workerExpanded branch February 24, 2016 18:10

gianm reviewed Mar 3, 2016
View reviewed changes

gianm added a commit to gianm/druid that referenced this pull request Mar 3, 2016

OverlordResource: Use getZkWorkers on RemoteTaskRunner.

e9c23bf

Restores old behavior of this api, from before apache#2249 when getWorkers returned ZkWorkers.

gianm mentioned this pull request Mar 3, 2016

OverlordResource: Use getZkWorkers on RemoteTaskRunner. #2585

Merged

nishantmonu51 mentioned this pull request Mar 3, 2016

Use ImmutableWorkerInfo instead of ZKWorker #2588

Merged

Conversation

drcrallen commented Jan 11, 2016

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!

gianm commented Feb 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drcrallen commented Feb 16, 2016

Uh oh!

drcrallen commented Feb 16, 2016

Uh oh!

drcrallen commented Feb 16, 2016

Uh oh!

drcrallen commented Feb 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drcrallen commented Feb 19, 2016

Uh oh!

gianm commented Feb 19, 2016

Uh oh!

drcrallen commented Feb 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nishantmonu51 commented Feb 23, 2016

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!

4 participants