Fix bug in TaskStorageQueryAdapter#16750
Merged
kfaraz merged 5 commits intoapache:masterfrom Jul 17, 2024
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| TaskStorageQueryAdapter taskStorageQueryAdapter = EasyMock.createMock(TaskStorageQueryAdapter.class); | ||
| EasyMock.expect(taskStorageQueryAdapter.getStatus("task")) | ||
| TaskQueryTool taskQueryTool = EasyMock.createMock(TaskQueryTool.class); |
Check notice
Code scanning / CodeQL
Possible confusion of local and field
Contributor
Author
|
Marked as dev blocker since master branch is broken after merging #16309 Note: The original bug fix in this PR does not address the broken master branch. |
AmatyaAvadhanula
approved these changes
Jul 17, 2024
Contributor
AmatyaAvadhanula
left a comment
AmatyaAvadhanula
left a comment
There was a problem hiding this comment.
LGTM! Thanks, @kfaraz
Contributor
This seems misleading as the bug in TaskStorageQueryAdapter doesn't break master. Could you please edit the PR description accordingly? |
Contributor
Author
|
Thanks for the quick review, @AmatyaAvadhanula ! |
edgar2020
pushed a commit
to edgar2020/druid
that referenced
this pull request
Jul 19, 2024
Changes: - Do not hold a reference to `TaskQueue` in `TaskStorageQueryAdapter` - Use `TaskStorage` instead of `TaskStorageQueryAdapter` in `IndexerMetadataStorageAdapter` - Rename `TaskStorageQueryAdapter` to `TaskQueryTool` - Fix newly added task actions `RetrieveUpgradedFromSegmentIds` and `RetrieveUpgradedToSegmentIds` by removing `isAudited` method.
edgar2020
pushed a commit
to edgar2020/druid
that referenced
this pull request
Jul 19, 2024
Changes: - Do not hold a reference to `TaskQueue` in `TaskStorageQueryAdapter` - Use `TaskStorage` instead of `TaskStorageQueryAdapter` in `IndexerMetadataStorageAdapter` - Rename `TaskStorageQueryAdapter` to `TaskQueryTool` - Fix newly added task actions `RetrieveUpgradedFromSegmentIds` and `RetrieveUpgradedToSegmentIds` by removing `isAudited` method.
sreemanamala
pushed a commit
to sreemanamala/druid
that referenced
this pull request
Aug 6, 2024
Changes: - Do not hold a reference to `TaskQueue` in `TaskStorageQueryAdapter` - Use `TaskStorage` instead of `TaskStorageQueryAdapter` in `IndexerMetadataStorageAdapter` - Rename `TaskStorageQueryAdapter` to `TaskQueryTool` - Fix newly added task actions `RetrieveUpgradedFromSegmentIds` and `RetrieveUpgradedToSegmentIds` by removing `isAudited` method.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
1. Bug in TaskStorageQueryAdapter
#15377 introduced a change to fetch task payloads from memory.
But it keeps a reference to the
TaskQueuewhenTaskStorageQueryAdapteris being constructed.The
TaskQueueobject is recreated upon every leader election.So, effectively, if a node becomes a leader again, it would use the older instance of the
TaskQueuefrom when it had first become the leader.
But that old instance of
TaskQueuewould not have information of new tasks and would eventually fall backto fetching stuff from metadata storage, rendering the caching layer useless.
Also, memory leaks.
2. Broken master
#16309 removed the method
TaskAction.isAudited(). Some task actions still have this method causing compilation failure.Changes
TaskQueueinTaskStorageQueryAdapterTaskStorageinstead ofTaskStorageQueryAdapterinIndexerMetadataStorageAdapterTaskStorageQueryAdaptertoTaskQueryToolRetrieveUpgradedFromSegmentIdsandRetrieveUpgradedToSegmentIdsby removing
isAuditedmethod.This PR has: