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.
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
Change EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS #1491
base: master
Are you sure you want to change the base?
Change EXIT_PROCESSOR_SLA_MARGIN to EXIT_PROCESSOR_SLA_SECONDS #1491
Changes from 2 commits
deab148
6174271
db4f1a2
85fc0f5
da7964a
8e9a537
a250c93
a0c7d4f
e074347
b2157c9
dc32c7d
9e42630
43bd892
b4c5342
545034d
3d60044
c8ea7aa
b25e4dc
1ba1a3d
872f1b5
67d08db
eac77e0
84b4472
4a98cda
2b663c2
99cf072
8c072f9
719e7b2
34cd035
d11ba3e
49587d8
96d9e31
b64b5eb
c1a0770
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Not sure should the value reflect the original value, which I think should be 10 block * 15s per block = 150?
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 guess this only takes place on tests. @souradeep-das can we confirm this - raise a value and whether tests fails?
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.
true, I think these are for tests. Fails when sla_seconds is >10
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.
if this is for tests then let's remove it. we should also remove the default setting below:
sla_seconds \\ @default_sla_seconds
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.
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.
the function spreads over multiple lines, let's allow it to do so.
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.
👍
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.
Only here we introduce this antipattern ;)
Please restore it as init param - which will be set where others are - and pass it to the Core's init (Core module serves usually as GenServer state). So you could retrieve its value as you do with
sla_seconds
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.
We're moving away from single pipes. Could you update
invalid_exits |> Enum.filter(fn...
toEnum.filter(invalid_exits, fn...
? Thanks!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.
Is this a good way or should the timestamp of each exit be stored in the DB and retrieved here to check if sla_seconds is elapsed
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 this is fine for this PR since it's still the existing behaviour, but could you maybe raise this discussion separately in the engineering channel?
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.
will change the logic to
exit_timestamp + sla_seconds <= time_now
when #1495 is mergedThere 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.
Note that before merging this PR, we'll need to update our infra to have this environment variable so we don't break the CI/CD environment.
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.
Too verbose I think :)
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.
Restore this and pass through as I suggested in
exit_processor/standard_exit.ex
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.
Is this fine? 🙇
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.
Looks fine to me, we're really waiting for the seconds to come
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.
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.
done
96d9e31