Fix loading lookup extension #17212
Merged
Fix loading lookup extension #17212
Conversation
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
TessaIO
force-pushed
the
fix-loading-lookup-extension
branch
from
2ecfa47
to
d9e4cd9
Compare
|
@cryptoe @LakshSingla @kfaraz Can you guys take a quick look at this? |
TessaIO
force-pushed
the
fix-loading-lookup-extension
branch
from
d9e4cd9
to
4339c87
Compare
|
@abhishekrb19 can you please re-run the pipeline one more time? |
Akshat-Jain
reviewed
Oct 9, 2024
...s-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java
Outdated
Show resolved
Hide resolved
...s-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java
Outdated
Show resolved
Hide resolved
...s-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java
Show resolved
Hide resolved
TessaIO
force-pushed
the
fix-loading-lookup-extension
branch
from
4339c87
to
c8772ae
Compare
|
@Akshat-Jain thanks for the review! PR updated, can you give it one more round? |
...re/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java
Show resolved
Hide resolved
...re/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java
Outdated
Show resolved
Hide resolved
...re/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java
Show resolved
Hide resolved
...re/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java
Outdated
Show resolved
Hide resolved
Akshat-Jain
reviewed
Oct 15, 2024
...s-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java
Outdated
Show resolved
Hide resolved
Signed-off-by: TessaIO <[email protected]>
TessaIO
force-pushed
the
fix-loading-lookup-extension
branch
from
c8772ae
to
9e1c412
Compare
|
@Akshat-Jain @abhishekrb19 Thanks for the review. One more round please. |
There was a problem hiding this comment.
@TessaIO a final set of comments. Looks good otherwise, thanks!
Also, we don't typically force push changes to make reviewing easier. The commits will be squashed and merged as one when committing to master.
...s-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java
Outdated
Show resolved
Hide resolved
...re/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java
Outdated
Show resolved
Hide resolved
...s-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java
Outdated
Show resolved
Hide resolved
...re/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java
Outdated
Show resolved
Hide resolved
...re/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: TessaIO <[email protected]>
|
@abhishekrb19 PR updated! |
Akshat-Jain
approved these changes
Oct 15, 2024
abhishekrb19
approved these changes
Oct 16, 2024
There was a problem hiding this comment.
Thanks for your first contribution @TessaIO, and thanks to @Akshat-Jain for the in-depth review!
@TessaIO, do you mind merging the latest master into this branch to get a clean CI run? I can commit this change after that.
…ading-lookup-extension
|
@abhishekrb19 done |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #17337.
Description
This PR aims to change the behavior of the druid lookups-cached-single extension. In fact, we aim to introduce the option to iterate over fetched data from the dataFetcher. We need this in the case of loadingLookups, that's why these changes don't impact the pollingLookup feature.
I also added the handling of a use case where the data exists in Druid but not in the actual data fetcher, which is in our use-case JDBC Data fetcher.
Release note
Key changed/added classes in this PR
Support iterating over fetched data for the loadinglookups for the druid lookups-cached-single extensionThis PR has: