[FLINK-24861][connector][jdbc] Fix false cache lookup for empty data #17754

gaurav726 · 2021-11-10T13:48:10Z

What is the purpose of the change

Ideally, in case of cache miss for a key, or with null value fetch for key, key shouldn't be cached, however empty result query should also be in the scope, so including an extra optional parameter to configure same

Brief change log

empty check on row list will make sure for database read for empty data for the key

Verifying this change

This change is a trivial check / code fix without any test coverage.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no/ don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2021-11-10T13:50:43Z

CI report:

20ea72f Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

flinkbot · 2021-11-10T13:53:00Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 29cfbf7 (Wed Nov 10 13:53:00 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

hililiwei · 2021-11-11T04:37:24Z

look good

wenlong88

@gaurav726 thanks for the contribution, I left some comments

wenlong88 · 2021-11-15T01:19:37Z

...or-jdbc/src/test/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableFactoryTest.java

@@ -174,6 +175,7 @@ public void testJdbcLookupProperties() {
                        .setCacheMaxSize(1000)
                        .setCacheExpireMs(10_000)
                        .setMaxRetryTimes(10)
+                        .setExcludeEmptyQueryResult(true)


you need to a new case instead of override the default value

you mean new test case,right?, I have added another one,

wenlong88 · 2021-11-15T01:22:05Z

...ctor-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcRowDataLookupFunction.java

-                for (RowData cachedRow : cachedRows) {
-                    collect(cachedRow);
+                if (!cachedRows.isEmpty() || !excludeEmptyQueryResult) {
+                    for (RowData cachedRow : cachedRows) {


i am afraid that you should not just ignore the empty values, instead you should remove the keyRow from the cache if exlucdeEmptyQueryResult is true, and collect is still needed.

Hi @wenlong88 , changes has been done, please review

Hi @wenlong88 , pipeline got succeed, can you please review and merge

hi, you need to add a test cover the cache changes, refer to JdbcRowDataLookupFunctionTest#testEval, on the other hands, I think it is more clearer if you do not put empty to cache in eval instead of put to cache and then invalidate.

cool, got it, making changes

@wenlong88 can we resolve conversations now? Also will @wuchong get it merge?

@wenlong88 anything else is remaining in this?

@gaurav726 the pr looks good for me now, but you still need to wait for Committers to join the review.

cool, thanks for the update @wenlong88

@wuchong can you please review my PR.
cc @wenlong88

wenlong88 · 2021-11-16T10:31:59Z

LGTM for now, cc @wuchong

wuchong · 2021-11-23T09:50:36Z

Thanks @gaurav726 for the great work. The implementation looks good to me. However, I have some concerns about the option key. From my point of view, this issue wants to disable caching missing key (same problem with FLINK-21415). Therefore, this option key would be better to belong to lookup.cache.xx. How about naming it lookup.cache.caching-missing-key=true which looks more intuitive?

gaurav726 · 2021-11-23T10:18:59Z

Hi @wuchong , thanks for reviewing my PR, making changes

gaurav726 · 2021-11-23T11:38:43Z

Hi @wuchong , changes has been done, please review, pipeline is running, will update once success

…via extra config option

gaurav726 · 2021-11-24T03:32:02Z

Hi @wuchong, pipeline got succeed, please review and merge

wuchong

LGTM.

… for lookup cache (apache#17754)

… for lookup cache (apache#17754) (cherry picked from commit f8fdb67)

rmetzger added the component=Connectors/JDBC label Nov 10, 2021

gaurav726 closed this Nov 12, 2021

gaurav726 reopened this Nov 12, 2021

gaurav726 closed this Nov 12, 2021

gaurav726 reopened this Nov 12, 2021

wenlong88 reviewed Nov 15, 2021

View reviewed changes

[FLINK-24861][connector][jdbc] Fix false cache lookup for empty data …

20ea72f

…via extra config option

wuchong approved these changes Nov 24, 2021

View reviewed changes

wuchong merged commit f8fdb67 into apache:master Nov 24, 2021

niklassemmler pushed a commit to niklassemmler/flink that referenced this pull request Feb 3, 2022

[FLINK-24861][connector][jdbc] Support to disable caching missing key…

1552cbc

… for lookup cache (apache#17754)

zhongqishang pushed a commit to zhongqishang/flink that referenced this pull request Mar 25, 2022

[FLINK-24861][connector][jdbc] Support to disable caching missing key…

f5545f6

… for lookup cache (apache#17754) (cherry picked from commit f8fdb67)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-24861][connector][jdbc] Fix false cache lookup for empty data #17754

[FLINK-24861][connector][jdbc] Fix false cache lookup for empty data #17754

gaurav726 commented Nov 10, 2021 •

edited

flinkbot commented Nov 10, 2021 •

edited

flinkbot commented Nov 10, 2021

hililiwei commented Nov 11, 2021

wenlong88 left a comment

wenlong88 Nov 15, 2021

gaurav726 Nov 15, 2021

wenlong88 Nov 15, 2021

gaurav726 Nov 15, 2021

gaurav726 Nov 15, 2021

wenlong88 Nov 15, 2021

gaurav726 Nov 15, 2021

gaurav726 Nov 16, 2021 •

edited

gaurav726 Nov 16, 2021

wenlong88 Nov 17, 2021

gaurav726 Nov 17, 2021

gaurav726 Nov 22, 2021 •

edited

wenlong88 commented Nov 16, 2021

wuchong commented Nov 23, 2021

gaurav726 commented Nov 23, 2021

gaurav726 commented Nov 23, 2021

gaurav726 commented Nov 24, 2021

wuchong left a comment

[FLINK-24861][connector][jdbc] Fix false cache lookup for empty data #17754

[FLINK-24861][connector][jdbc] Fix false cache lookup for empty data #17754

Conversation

gaurav726 commented Nov 10, 2021 • edited

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Nov 10, 2021 • edited

CI report:

flinkbot commented Nov 10, 2021

Automated Checks

Review Progress

hililiwei commented Nov 11, 2021

wenlong88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaurav726 Nov 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaurav726 Nov 22, 2021 • edited

Choose a reason for hiding this comment

wenlong88 commented Nov 16, 2021

wuchong commented Nov 23, 2021

gaurav726 commented Nov 23, 2021

gaurav726 commented Nov 23, 2021

gaurav726 commented Nov 24, 2021

wuchong left a comment

Choose a reason for hiding this comment

gaurav726 commented Nov 10, 2021 •

edited

flinkbot commented Nov 10, 2021 •

edited

gaurav726 Nov 16, 2021 •

edited

gaurav726 Nov 22, 2021 •

edited