Skip to content

Implemented a new endpoint to get running tasks by datasource#5260

Merged
gianm merged 1 commit intoapache:masterfrom
NirajaM:feature-1376-niraja
Mar 13, 2018
Merged

Implemented a new endpoint to get running tasks by datasource#5260
gianm merged 1 commit intoapache:masterfrom
NirajaM:feature-1376-niraja

Conversation

@NirajaM
Copy link
Copy Markdown
Contributor

@NirajaM NirajaM commented Jan 16, 2018 •
edited
Loading

  • added datasource information as part of existing endpoint /druid/indexer/v1/runningTasks.
  • Added junit test cases and fixed existing test cases
  • Fixed review comments

Use case for us - We have multiple jobs running for different datasources, so we want to filter running tasks only for particular datasource in the console.

@NirajaM NirajaM mentioned this pull request Jan 16, 2018
Closed
nishantmonu51
nishantmonu51 reviewed Jan 16, 2018
View reviewed changes
Comment thread ...a-indexing-service/src/test/java/io/druid/indexing/kafka/supervisor/KafkaSupervisorTest.java Outdated
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Jan 16, 2018

Choose a reason for hiding this comment

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

formatting

Comment thread indexing-service/src/main/java/io/druid/indexing/overlord/ForkingTaskRunner.java Outdated
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Jan 16, 2018

Choose a reason for hiding this comment

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

formatting

Copy link
Copy Markdown
Contributor Author

@NirajaM NirajaM Jan 17, 2018

Choose a reason for hiding this comment

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

done

Comment thread indexing-service/src/main/java/io/druid/indexing/overlord/RemoteTaskRunnerWorkItem.java Outdated
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Jan 16, 2018

Choose a reason for hiding this comment

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

final ?

Comment thread indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java Outdated
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Jan 16, 2018

Choose a reason for hiding this comment

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

formatting

Copy link
Copy Markdown
Contributor Author

@NirajaM NirajaM Jan 17, 2018

Choose a reason for hiding this comment

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

done

Comment thread indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java Outdated
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Jan 16, 2018

Choose a reason for hiding this comment

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

maybe add a method to TaskStorageQueryAdapter getCreatedDateTimeAndDataSource which fetches datasource and created time in single query instead of issuing multiple queries to metadata storage.

Copy link
Copy Markdown
Contributor Author

@NirajaM NirajaM Jan 17, 2018

Choose a reason for hiding this comment

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

Added getCreatedDateTimeAndDataSource in TaskStorageQueryAdapter

@NirajaM NirajaM force-pushed the feature-1376-niraja branch 3 times, most recently from 2e06749 to 09c2a6e Compare January 18, 2018 07:24
@NirajaM NirajaM changed the title As part of this feature, implemented a new endpoint to get running ta… Implemented a new endpoint to get running tasks by datasource Jan 18, 2018
@NirajaM NirajaM force-pushed the feature-1376-niraja branch 3 times, most recently from fd75793 to 0795e1a Compare January 18, 2018 11:03
@NirajaM
Copy link
Copy Markdown
Contributor Author

NirajaM commented Jan 18, 2018

@nishantmonu51 One test case is failing from RemoteTaskRunnerTest class due to timeout exception. Due to this build is not getting passed. Any suggestion on this ?

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Jan 19, 2018

@NirajaM I'll try re-running the test

@nishantmonu51
Copy link
Copy Markdown
Member

nishantmonu51 commented Jan 22, 2018

Travis failure stack trace -

java.lang.AssertionError: runningTasks expected:<[first, second]> but was:<[]>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:743)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at io.druid.indexing.overlord.RemoteTaskRunnerTest.testBootstrap(RemoteTaskRunnerTest.java:354)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)

@NirajaM: this is probably related to your changes, can you please check,
I am restarting travis check again to check one more time.

nishantmonu51
nishantmonu51 reviewed Jan 22, 2018
View reviewed changes
Comment thread indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java Outdated
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Jan 22, 2018

Choose a reason for hiding this comment

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

this still calls the apply twice which leads to multiple queries to the db.

@NirajaM NirajaM force-pushed the feature-1376-niraja branch 11 times, most recently from 5413e32 to 1e73a14 Compare January 23, 2018 12:56
@NirajaM
Copy link
Copy Markdown
Contributor Author

NirajaM commented Jan 23, 2018

@nishantmonu51 Fixed junit test case and the review comment. Could you please review

nishantmonu51
nishantmonu51 approved these changes Jan 24, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 left a comment

Choose a reason for hiding this comment

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

LGTM, left a minor comment. 👍 post travis.

Comment thread indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java Outdated
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 Jan 24, 2018

Choose a reason for hiding this comment

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

minor nit: you can directly call taskStorageQueryAdapter.get instead of creating cTDsFunction

Copy link
Copy Markdown
Contributor Author

@NirajaM NirajaM Jan 25, 2018

Choose a reason for hiding this comment

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

Thanks. Fixed the same

@NirajaM NirajaM force-pushed the feature-1376-niraja branch 4 times, most recently from 5de2317 to 4a8bc0f Compare January 25, 2018 08:29
@NirajaM NirajaM force-pushed the feature-1376-niraja branch 8 times, most recently from 043dee4 to fbc593a Compare February 26, 2018 12:02
@NirajaM
Copy link
Copy Markdown
Contributor Author

NirajaM commented Feb 26, 2018 •
edited
Loading

@drcrallen Can we do something to fix this timeout exceptions. Most of the time one or the other test case fails due to java.lang.RuntimeException: java.util.concurrent.TimeoutException. When I check locally everything is fine.

If you keep on triggering build sometimes it passes. I think we should first fix this.
I am ready to fix this. Please suggest me.

@NirajaM NirajaM force-pushed the feature-1376-niraja branch 11 times, most recently from 0d79b40 to aae2f97 Compare February 27, 2018 10:20
drcrallen
drcrallen approved these changes Mar 8, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@drcrallen drcrallen left a comment

Choose a reason for hiding this comment

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

Cool, thanks! please fix merge conflicts and this is good to go

@NirajaM
As part of this feature, implemented a new endpoint to get running ta…
a789d3b 
…sks by datasources

and added datasource information as part of existing endpoint /druid/indexer/v1/runningTasks.

Added junit test cases for the newly implemented API and fixed existing junit test cases.

Fixed review comments - added new method getCreatedDateTimeAndDataSource into TaskStorageQueryAdapter class
and formatted changed files
@NirajaM
Copy link
Copy Markdown
Contributor Author

NirajaM commented Mar 12, 2018

@drcrallen Thanks. Fixed the merge conflicts

gianm
gianm approved these changes Mar 13, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM. @NirajaM could you please fill out the project CLA so we can accept this contribution?

It's at: http://druid.io/community/cla.html

@NirajaM
Copy link
Copy Markdown
Contributor Author

NirajaM commented Mar 13, 2018

@gianm I already signed and sent project CLA

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Mar 13, 2018

@NirajaM I don't see it on the list we have that keeps track of who has filled out the online form. Did you fill that form out or did you send something over email? If you had done an email, it might be easier to fill out the form since I am not totally sure where the emails go.

@NirajaM
Copy link
Copy Markdown
Contributor Author

NirajaM commented Mar 13, 2018

@gianm I filled the online form as well

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Mar 13, 2018

@NirajaM Okay I see it now. Thank you.

@jihoonson jihoonson mentioned this pull request Mar 20, 2018
Closed
@jihoonson jihoonson mentioned this pull request Nov 30, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gianm gianm gianm approved these changes

@nishantmonu51 nishantmonu51 nishantmonu51 approved these changes

@drcrallen drcrallen drcrallen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.13.0

Development

Successfully merging this pull request may close these issues.

6 participants

@NirajaM @gianm @nishantmonu51 @himanshug @drcrallen @dclim