-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-28417][table] Add interfaces for LookupCache and default implementation #20196
Conversation
8c7771d
to
247f76a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank @PatrickRen for the contribution. I left some comments.
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Outdated
Show resolved
Hide resolved
...table-common/src/main/java/org/apache/flink/table/connector/source/lookup/LookupOptions.java
Outdated
Show resolved
Hide resolved
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Outdated
Show resolved
Hide resolved
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Show resolved
Hide resolved
@wuchong Thanks for the review! I addresses your comments in the latest commit. Please take a look when you are available. |
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Outdated
Show resolved
Hide resolved
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PatrickRen, thanks for your contribution! I've left some minor comments, please check.
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Outdated
Show resolved
Hide resolved
...table-common/src/main/java/org/apache/flink/table/connector/source/lookup/LookupOptions.java
Outdated
Show resolved
Hide resolved
...table-common/src/main/java/org/apache/flink/table/connector/source/lookup/LookupOptions.java
Outdated
Show resolved
Hide resolved
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Outdated
Show resolved
Hide resolved
code(LookupCacheType.FULL.toString())) | ||
.build()); | ||
|
||
public static final ConfigOption<Integer> MAX_RETRIES = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better not to add this new option if we have no migration plan for existing connectors (e.g., hbase & jdbc), and it is not related with caching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we do plan to migrate hbase and jdbc to the new framework, so I prefer to keep this option.
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Outdated
Show resolved
Hide resolved
1. Change serial ID to 1 2. Sanity check in constructor 3. Always register metrics in DefaultLookupCache#open 4. Change default value of max retries to 3
@wuchong @SmirAlex @lincoln-lil Thank you all for the review! I updated the PR to address your comments. Please take another look at your convenience. |
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Show resolved
Hide resolved
...n/src/main/java/org/apache/flink/table/connector/source/lookup/cache/DefaultLookupCache.java
Outdated
Show resolved
Hide resolved
2. make error message more descriptive in DefaultLookupCache.fromConfig 3. Add test for builder and fromConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
…o enable mongodb connector to run with decodable-flink (apache#54) * [FLINK-28417][table] Add interfaces for LookupCache and default implementation (apache#20196) * manually add ThreadSafeSimpleCounter, which is the only missing dependency of LookupCache/DefaultLookupCache that is not in flink 1.15 --------- Co-authored-by: Qingsheng Ren <[email protected]>
What is the purpose of the change
This pull request introduces interfaces for LookupCache and CacheMetricGroup, also a default implementation of LookupCache.
Brief change log
LookupCache
andCacheMetricGroup
LookupCache
asDefaultLookupCache
CacheMetricGroup
asInternalCacheMetricGroup
Verifying this change
This change added unit tests for the newly added
DefaultLookupCache
andInternalCacheMetricGroup
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation