-
Notifications
You must be signed in to change notification settings - Fork 528
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
Adding total timeout to task models #313
Merged
shaileshpadave
merged 6 commits into
conductor-oss:main
from
shaileshpadave:addTotalTimeout
Nov 27, 2024
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,6 +127,10 @@ public enum RetryLogic { | |
@ProtoField(id = 21) | ||
private String baseType; | ||
|
||
@ProtoField(id = 22) | ||
@NotNull | ||
private long totalTimeoutSeconds; | ||
|
||
private SchemaDef inputSchema; | ||
private SchemaDef outputSchema; | ||
private boolean enforceSchema; | ||
|
@@ -464,6 +468,14 @@ public void setEnforceSchema(boolean enforceSchema) { | |
this.enforceSchema = enforceSchema; | ||
} | ||
|
||
public long getTotalTimeoutSeconds() { | ||
return totalTimeoutSeconds; | ||
} | ||
|
||
public void setTotalTimeoutSeconds(@NotNull long totalTimeoutSeconds) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: a |
||
this.totalTimeoutSeconds = totalTimeoutSeconds; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return name; | ||
|
@@ -497,7 +509,8 @@ && getRetryLogic() == taskDef.getRetryLogic() | |
&& Objects.equals(getOwnerEmail(), taskDef.getOwnerEmail()) | ||
&& Objects.equals(getBaseType(), taskDef.getBaseType()) | ||
&& Objects.equals(getInputSchema(), taskDef.getInputSchema()) | ||
&& Objects.equals(getOutputSchema(), taskDef.getOutputSchema()); | ||
&& Objects.equals(getOutputSchema(), taskDef.getOutputSchema()) | ||
&& Objects.equals(getTotalTimeoutSeconds(), taskDef.getTotalTimeoutSeconds()); | ||
} | ||
|
||
@Override | ||
|
@@ -523,6 +536,7 @@ public int hashCode() { | |
getOwnerEmail(), | ||
getBaseType(), | ||
getInputSchema(), | ||
getOutputSchema()); | ||
getOutputSchema(), | ||
getTotalTimeoutSeconds()); | ||
} | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,28 @@ public void testTaskDef() { | |
assertTrue(validationErrors.contains("ownerEmail cannot be empty")); | ||
} | ||
|
||
@Test | ||
public void testTaskDefTotalTimeOutSeconds() { | ||
TaskDef taskDef = new TaskDef(); | ||
taskDef.setName("test-task"); | ||
taskDef.setRetryCount(1); | ||
taskDef.setTimeoutSeconds(1000); | ||
taskDef.setTotalTimeoutSeconds(900); | ||
taskDef.setResponseTimeoutSeconds(1); | ||
taskDef.setOwnerEmail("[email protected]"); | ||
|
||
Set<ConstraintViolation<Object>> result = validator.validate(taskDef); | ||
assertEquals(1, result.size()); | ||
|
||
List<String> validationErrors = new ArrayList<>(); | ||
result.forEach(e -> validationErrors.add(e.getMessage())); | ||
|
||
assertTrue( | ||
validationErrors.toString(), | ||
validationErrors.contains( | ||
"TaskDef: test-task timeoutSeconds: 1000 must be less than or equal to totalTimeoutSeconds: 900")); | ||
} | ||
|
||
@Test | ||
public void testTaskDefInvalidEmail() { | ||
TaskDef taskDef = new TaskDef(); | ||
|
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be added to
equals
,hashcode
andtoString
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@v1r3n
hashCode
is consideringtaskDefName
Or do you want to use just "task name"?
PROS
CONS
equals
: if only id is used in hashCode but equals considers additional fields, the contract will be broken.assertNotEquals(task0, task1)
. If both task have the same name but different fields, the assertion istrue
now but will befalse
is we just consider task name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andhashCode