Retry the setup of an ingestion reader when it fails - #2806
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.5 #2806 +/- ##
===================================================
+ Coverage 75.77% 75.80% +0.03%
===================================================
Files 200 200
Lines 13922 13925 +3
===================================================
+ Hits 10549 10556 +7
+ Misses 3363 3359 -4
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
5ad48bb to
bd6999f
Compare
| error: err, | ||
| }); | ||
| // the source may have been removed from the configuration | ||
| // while its setup was in progress |
There was a problem hiding this comment.
this comment is misleading: it is not that it "may have been removed", it is certain that is was removed c.f. the beginning of the function:
const newReaders = this.logReadersUpdate;
this.logReadersUpdate = [];
→ here you want to explain why you re-add it
| // while its setup was in progress | |
| // Setup failures are usually transient (source unreachable, | |
| // invalid credentials...): queue the reader again so that it | |
| // is retried on the next cycle |
There was a problem hiding this comment.
Reworded to say why the reader is queued again.
One precision : that comment was about _ingestionSources, the registry of configured sources, not about logReadersUpdate which the function drains at the top. Those are two different structures, and the guard reads the former.
It was imprecise for another reason though. _closeLogState only deletes the _ingestionSources entry when it finds the reader in logReadersUpdate or in logReaders, and while its setup is in progress the reader is in neither, so a source removed right then still leaves its entry behind. What the guard actually catches is a reader that is no longer the one registered for its bucket, which is what the new comment says.
| // Setup failures are usually transient (source unreachable, | ||
| // invalid credentials...). Queue the reader again so that it | ||
| // is retried on the next cycle: `applyUpdates` only refreshes | ||
| // sources that are already registered, so a reader dropped | ||
| // here would never be set up again. |
There was a problem hiding this comment.
not relevant to the code: the old comment was -and still is- correct, we don't add this one to logReaders and continue setting up others...
| // Setup failures are usually transient (source unreachable, | |
| // invalid credentials...). Queue the reader again so that it | |
| // is retried on the next cycle: `applyUpdates` only refreshes | |
| // sources that are already registered, so a reader dropped | |
| // here would never be set up again. | |
| // if setup fails for a log reader, don't add it to `logReaders` | |
| // log the error and continue setting up others |
| const zenkoBucket = logReader.getTargetZenkoBucketName(); | ||
| this.log.error('error setting up log reader, retrying later', { | ||
| method: 'IngestionPopulator._setupUpdatedReaders', | ||
| zenkoBucket, |
There was a problem hiding this comment.
I think we should log the location instead of the bucket, seems more appropriate - or in addition to the bucket, for consistency with other logs in this file:
| const zenkoBucket = logReader.getTargetZenkoBucketName(); | |
| this.log.error('error setting up log reader, retrying later', { | |
| method: 'IngestionPopulator._setupUpdatedReaders', | |
| zenkoBucket, | |
| this.log.error('error setting up log reader, retrying later', { | |
| method: 'IngestionPopulator._setupUpdatedReaders',) | |
| zenkoBucket: logReader.getTargetZenkoBucketName(), | |
| location: logReader.getLocationConstraint(), |
There was a problem hiding this comment.
Done, location sits next to zenkoBucket now. I kept the zenkoBucket const since the guard right below reads it.
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
not the right target branch: no point fixing so far back in time: from 9.5 is enough |
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
When a reader's setup fails (typically the getRaftId call to the source), it was dropped from the pending list and never set up again: the reader stays registered in _ingestionSources, so applyUpdates considers the bucket already handled and only refreshes it. The bucket was therefore left out of ingestion until the pod was restarted. Queue the reader again instead, so it is retried on the next cycle. This follows what _processLogReaderEntries already does with read failures, and keeps the other locations ingesting, which restarting the pod on a failed healthcheck would not. The reader is only queued again if it is still the one registered for its bucket, so a source removed or replaced in the meantime is not brought back. Issue: BB-846
bd6999f to
2f9c7f5
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
Retargeted to On the branch prefix: BB-846 is already typed Bug in Jira, only the branch name says otherwise. Renaming it means closing this PR and opening a new one, which also moves the base of #2821 stacked on top, so I left the branch name as is. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
|
What happens when the location is deleted? Please confirm there no risk of path or race condition where we would keep retrying to setup a location which was removed... |
3 participants
When a reader's setup fails (typically the getRaftId call to the source), it was dropped from the pending list and never set up again: the reader stays registered in _ingestionSources, so applyUpdates considers the bucket already handled and only refreshes it. The bucket was therefore left out of ingestion until the pod was restarted.
Queue the reader again instead, so it is retried on the next cycle. This follows what _processLogReaderEntries already does with read failures, and keeps the other locations ingesting, which restarting the pod on a failed healthcheck would not.
The reader is only queued again if its source is still configured, as it may have been removed while its setup was in progress.
Issue: BB-846