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

Add start time and duration to tasks #16829

Merged
merged 1 commit into from Feb 27, 2016

Conversation

Projects
None yet
2 participants
@imotov
Member

imotov commented Feb 26, 2016

Tasks now contain timestamps indicating when the tasks were created and current run time

@nik9000

View changes

Show outdated Hide outdated ...ava/org/elasticsearch/action/admin/cluster/node/tasks/list/TaskInfo.java
@nik9000

View changes

Show outdated Hide outdated ...ava/org/elasticsearch/action/admin/cluster/node/tasks/list/TaskInfo.java
@nik9000

View changes

Show outdated Hide outdated ...search/action/admin/cluster/node/tasks/RecordingTaskManagerListener.java
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Feb 26, 2016

Contributor

Hurray! I left some minor comments, all of which you can feel free to say "no, I like it the way it is, thank you". I think it'd tell a great story if you added "get me tasks that have been running longer than X seconds" to the task list API in this PR. It'd make it super obvious why this is important. But this PR is fine as is if you want.

Contributor

nik9000 commented Feb 26, 2016

Hurray! I left some minor comments, all of which you can feel free to say "no, I like it the way it is, thank you". I think it'd tell a great story if you added "get me tasks that have been running longer than X seconds" to the task list API in this PR. It'd make it super obvious why this is important. But this PR is fine as is if you want.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Feb 26, 2016

Contributor

To make it official, LGTM.

Contributor

nik9000 commented Feb 26, 2016

To make it official, LGTM.

@imotov

This comment has been minimized.

Show comment
Hide comment
@imotov

imotov Feb 26, 2016

Member

@nik9000 I pushed changes to address your comments. What do you think?

Member

imotov commented Feb 26, 2016

@nik9000 I pushed changes to address your comments. What do you think?

@@ -50,17 +51,24 @@
private final String description;
private final long startTime;

This comment has been minimized.

@nik9000

nik9000 Feb 26, 2016

Contributor

Maybe these should be TimeValues?

@nik9000

nik9000 Feb 26, 2016

Contributor

Maybe these should be TimeValues?

This comment has been minimized.

@nik9000

nik9000 Feb 26, 2016

Contributor

Err, the next one a TimeValue, this one a DateTime or something? Its not required, at all and I just thought of it but a TimeValue would make runningTimeNanos easier to read I think.

@nik9000

nik9000 Feb 26, 2016

Contributor

Err, the next one a TimeValue, this one a DateTime or something? Its not required, at all and I just thought of it but a TimeValue would make runningTimeNanos easier to read I think.

This comment has been minimized.

@imotov

imotov Feb 26, 2016

Member

This is just a transport format, which is easier to work with. We do convert this long to a TimeValue in XContentBuilder#timeValueField() if a user requests human readable output. It also happens to dates in XContentBuilder#dateValueField(). So, I don't think storing it here in milliseconds and nanoseconds will affect readability.

@imotov

imotov Feb 26, 2016

Member

This is just a transport format, which is easier to work with. We do convert this long to a TimeValue in XContentBuilder#timeValueField() if a user requests human readable output. It also happens to dates in XContentBuilder#dateValueField(). So, I don't think storing it here in milliseconds and nanoseconds will affect readability.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Feb 26, 2016

Contributor

Fair enough. LGTM.

Contributor

nik9000 commented Feb 26, 2016

Fair enough. LGTM.

Add start time and duration to tasks
Tasks now contain timestamps indicating when the tasks were created and current run time

@imotov imotov merged commit 863fab4 into elastic:master Feb 27, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@imotov imotov referenced this pull request Feb 29, 2016

Closed

Task Management #15117

12 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment