Adding total timeout to task models #313
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shaileshpadave
force-pushed
the
addTotalTimeout
branch
4 times, most recently
from
95851be to
039e8af
Compare
| @@ -202,6 +202,9 @@ public boolean isRetriable() { | |||
| @ProtoField(id = 42) | |||
| private boolean subworkflowChanged; | |||
|
|
|||
| @ProtoField(id = 43) | |||
| private long firstStartTime; | |||
There was a problem hiding this comment.
should be added to equals, hashcode and toString?
There was a problem hiding this comment.
Actually, I think we should change the hashCode to be aware of the task's PK -- which is taskName. Thoughts @jmigueprieto
There was a problem hiding this comment.
@v1r3n hashCode is considering taskDefName
@Override
public int hashCode() {
return Objects.hash(
getTaskType(),
getStatus(),
getInputData(),
getReferenceTaskName(),
getWorkflowPriority(),
getRetryCount(),
getSeq(),
getCorrelationId(),
getPollCount(),
getTaskDefName(),
...
}
Or do you want to use just "task name"?
PROS
- It will be simpler
- (small) Performance gain
CONS
- We'll need to modify
equals: if only id is used in hashCode but equals considers additional fields, the contract will be broken. - We'll have an incomplete equality: this might break existing code e.g.: tests that assert
assertNotEquals(task0, task1). If both task have the same name but different fields, the assertion istruenow but will befalseis we just consider task name.
There was a problem hiding this comment.
I think we can analyze this further in another PR. But, for the moment I believe that we should stick to considering the added fields to equals and hashCode
| /** | ||
| * @return the total timeout in seconds | ||
| */ | ||
| public long getTotalTimeoutSeconds() { |
| @@ -142,11 +146,17 @@ public TaskDef(String name, String description) { | |||
| this.description = description; | |||
| } | |||
|
|
|||
| public TaskDef(String name, String description, int retryCount, long timeoutSeconds) { | |||
| public TaskDef( | |||
There was a problem hiding this comment.
Can a new constructor be added instead of changing the signature of this one?
jmigueprieto
requested changes
Nov 22, 2024
shaileshpadave
force-pushed
the
addTotalTimeout
branch
from
c6aaddb to
659baf6
Compare
shaileshpadave
force-pushed
the
addTotalTimeout
branch
from
cc76d81 to
35fd264
Compare
shaileshpadave
force-pushed
the
addTotalTimeout
branch
2 times, most recently
from
cfd6396 to
31f4487
Compare
shaileshpadave
changed the title
shaileshpadave
force-pushed
the
addTotalTimeout
branch
from
31f4487 to
f978467
Compare
| return totalTimeoutSeconds; | ||
| } | ||
|
|
||
| public void setTotalTimeoutSeconds(@NotNull long totalTimeoutSeconds) { |
There was a problem hiding this comment.
NIT: a long (primitive type) cannot be null. The @NotNull is redundant.
jmigueprieto
approved these changes
Nov 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request type
Changes in this PR
Added new parameter total task timeout to taskmodel and task def. This will be useful to timeout task if it exceeds total timeout value (including timeout in retry as well), this PR is only for model changes. Core service changes will be in another PR.