Fix RFS Shutdown logic during exception cases and set kafka tests as isolated #1385
Conversation
Signed-off-by: Andre Kurait <[email protected]>
AndreKurait
had a problem deploying
to
migrations-cicd
GitHub Actions
Failure
Signed-off-by: Andre Kurait <[email protected]>
AndreKurait
temporarily deployed
to
migrations-cicd
GitHub Actions
Inactive
AndreKurait
changed the title
Signed-off-by: Andre Kurait <[email protected]>
AndreKurait
temporarily deployed
to
migrations-cicd
GitHub Actions
Inactive
| if (successorWorkItemIds.size() == 1 && workItemId.equals(successorWorkItemIds.get(0))) { | ||
| log.atWarn().setMessage("No real progress was made for work item: {}. Will retry with larger timeout").addArgument(workItemId).log(); |
There was a problem hiding this comment.
This is a strange case, it seems like the getSuccessorWorkItemIds should error out internally before returning up to this level. Can we rework this?
There was a problem hiding this comment.
getSuccessorWorkItemIds does error out, but this should really be a warn instead of an error.
The case here is that the lease is just long enough to send one request to the target cluster successfully, getSuccessorWorkItemIds does throw.
With the new try catch, this would be caught, but this isn't an "Error" case, more of a Warn which is why we shouldn't rely on that exception in getSuccessorWorkItemIds
| } else { | ||
| log.atWarn().setMessage("No progress cursor to create successor work items from. This can happen when" + | ||
| "downloading and unpacking shard takes longer than the lease").log(); | ||
| log.atWarn().setMessage("Skipping creation of successor work item to retry the existing one with more time") | ||
| .log(); | ||
| } |
There was a problem hiding this comment.
Nit: Can we invert the flow of control and return if the precondition fails right away?
There was a problem hiding this comment.
Ideally we'd have only one 'level' of if/elseif/else blocks for each function, makes it much cleaner to read.
AndreKurait
requested review from
chelma,
gregschohn,
jugal-chauhan,
lewijacn,
mikaylathompson and
sumobrian
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1385 +/- ##
============================
============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Description
Fix RFS Shutdown logic during exception cases
Set kafka tests as isolated
Remove deprecated usage of KafkaContainer in favor of ConfluentKafkaContainer
Issues Resolved
MIGRATIONS-2461
MIGRATIONS-2460
Testing
GHA
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.