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

sort task history updates by ExtendedTaskState enum ordinal, not timestamp #1188

Merged
merged 3 commits into from Aug 22, 2016

Conversation

Projects
None yet
4 participants
@ssalinas
Member

ssalinas commented Aug 2, 2016

I found a few cases where it could happen that what we would consider the 'current state' of the task, is not the update that has the latest timestamp:

  • Rapid updates in succession, causing the history updates to have identical or out-of-order timestamps
  • cleaning triggered early in task lifecycle, followed quickly by a starting/running update will indefinitely leave the task in that state (not allowing it to clean/shut down properly)

This PR updates the sort called when fetching task histories to sort by the ExtendedTaskSTate ordinal instead of relying only on timestamp. This should give us the correct 'current' state in all cases since the enum is defined in the order we expect statuses to progress during the task lifecycle.

/cc @tpetr @stevenschlansker @subratbasnet #1187

Show outdated Hide outdated ...Base/src/main/java/com/hubspot/singularity/SingularityTaskIdHistory.java
}
}
Collections.sort(updates, comparator);
SingularityTaskHistoryUpdate lastUpdate = Iterables.getLast(updates);

This comment has been minimized.

@stevenschlansker

stevenschlansker Aug 2, 2016

Contributor

Could consider using Collections.max here for a slight efficiency gain, also avoids mutating an argument (which is usually dicey code style IMO)

@stevenschlansker

stevenschlansker Aug 2, 2016

Contributor

Could consider using Collections.max here for a slight efficiency gain, also avoids mutating an argument (which is usually dicey code style IMO)

This comment has been minimized.

@ssalinas

ssalinas Aug 2, 2016

Member

👍 on Collections.max, updated

@ssalinas

ssalinas Aug 2, 2016

Member

👍 on Collections.max, updated

Show outdated Hide outdated ...Base/src/main/java/com/hubspot/singularity/SingularityTaskIdHistory.java
@@ -15,17 +18,20 @@
private final Optional<ExtendedTaskState> lastTaskState;
private final Optional<String> runId;
private static final Comparator<SingularityTaskHistoryUpdate> comparator = new Comparator<SingularityTaskHistoryUpdate>() {

This comment has been minimized.

@stevenschlansker

stevenschlansker Aug 2, 2016

Contributor

Should this be composed with a comparator over timestamp, in case a task enters the same state more than once, as a tiebreaker? or is that not possible?

@stevenschlansker

stevenschlansker Aug 2, 2016

Contributor

Should this be composed with a comparator over timestamp, in case a task enters the same state more than once, as a tiebreaker? or is that not possible?

This comment has been minimized.

@ssalinas

ssalinas Aug 2, 2016

Member

We can't have two separate state updates for the same state, so shouldn't need that

@ssalinas

ssalinas Aug 2, 2016

Member

We can't have two separate state updates for the same state, so shouldn't need that

@tpetr

This comment has been minimized.

Show comment
Hide comment
Member

tpetr commented Aug 2, 2016

(FYI @wsorenson)

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Aug 2, 2016

Member

Another fyi, the original change was introduced by me back in #932 when trying to improve task search speed. The list of updates being returned by a new way of fetching them was an unmodifiable list, so I had resorted to checking timestamps. Should have been checking ordinals instead there

Member

ssalinas commented Aug 2, 2016

Another fyi, the original change was introduced by me back in #932 when trying to improve task search speed. The list of updates being returned by a new way of fetching them was an unmodifiable list, so I had resorted to checking timestamps. Should have been checking ordinals instead there

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 4, 2016

I've tested this extensively yesterday with over a 100 tasks. The earlier problem did not recur. So looks like this working!

Thanks!

ghost commented Aug 4, 2016

I've tested this extensively yesterday with over a 100 tasks. The earlier problem did not recur. So looks like this working!

Thanks!

@ssalinas ssalinas modified the milestone: 0.10.0 Aug 4, 2016

@ssalinas ssalinas added the hs_stable label Aug 9, 2016

@ssalinas ssalinas modified the milestones: 0.10.0, 0.11.0, 0.10.1 Aug 19, 2016

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Aug 22, 2016

Member

Merging this for 0.10.1 bug fix release

Member

ssalinas commented Aug 22, 2016

Merging this for 0.10.1 bug fix release

@ssalinas ssalinas merged commit 1f028e2 into master Aug 22, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@ssalinas ssalinas deleted the out_of_order_updates branch Aug 22, 2016

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