Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[FLINK-36149][state] Make the RocksdbCompactFilter parameters take effect directly at the statebackend level #25257

Merged
merged 1 commit into from
Sep 5, 2024
Merged

[FLINK-36149][state] Make the RocksdbCompactFilter parameters take effect directly at the statebackend level #25257

merged 1 commit into from
Sep 5, 2024

Conversation

lexluo09
Copy link
Contributor

What is the purpose of the change

In Rank operators, add the cleanupInRocksdbCompactFilter(queryTimeAfterNumEntries) parameter setting, allowing adjustment based on the size of user state to enhance performance.

Brief change log

  • Add a configuration parameter, TABLE_EXEC_RANK_ROCKSDB_CLEANUP_QUERY_TIME_AFTER_NUM_ENTRIES, in StreamExecRank.
  • Update StateConfigUtil to support creating StateTtlConfig using retentionTime and queryTimeAfterNumEntries.
  • Support configuration of cleanupFullSnapshot and cleanupInRocksdbCompactFilter.

Verifying this change

This change is already covered by existing tests, such as AppendOnlyFirstNFunctionTest、TopNFunctionTestBase.

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

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

Documentation

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

@flinkbot
Copy link
Collaborator

flinkbot commented Aug 26, 2024 •
edited
Loading

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@lexluo09
Copy link
Contributor Author

@flinkbot run azure

@lexluo09
Copy link
Contributor Author

@flinkbot run azure

@lexluo09 lexluo09 changed the title [FLINK-36149][table]In Rank operators, add the cleanupInRocksdbCompactFilter(queryTimeAfterNumEntries) parameter settin [FLINK-36149][table] Make the RocksdbCompactFilter parameters take effect directly at the statebackend level Aug 28, 2024
lincoln-lil
lincoln-lil reviewed Aug 28, 2024
View reviewed changes
Copy link
Contributor

@lincoln-lil lincoln-lil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lexluo09 thanks for contributing this! Since the scope of the pr changes, the module name in the commit msg should be updated as well(table -> state).

...nner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecRank.java Outdated Show resolved Hide resolved
@lincoln-lil
Copy link
Contributor

cc @Zakelly @masteryhx since you're the expert in this area, would one of you help the review if you have time？

@lexluo09 lexluo09 changed the title [FLINK-36149][table] Make the RocksdbCompactFilter parameters take effect directly at the statebackend level [FLINK-36149][state] Make the RocksdbCompactFilter parameters take effect directly at the statebackend level Aug 28, 2024
@lexluo09
Copy link
Contributor Author

@lexluo09 thanks for contributing this! Since the scope of the pr changes, the module name in the commit msg should be updated as well(table -> state).

Thank you for helping with the review. I have made the necessary changes based on your feedback.

Zakelly
Zakelly requested changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lexluo09 Thanks for the PR! Please find my comments below:

.../main/java/org/apache/flink/contrib/streaming/state/ttl/RocksDbTtlCompactFiltersManager.java Outdated Show resolved Hide resolved
@Zakelly
Copy link
Contributor

Zakelly commented Aug 29, 2024

And BTW, you should run mvn package -Dgenerate-config-docs -pl flink-docs -am -nsu -DskipTests -Pskip-webui-build if you add/change the configurations. Please check the instructions in flink-docs/README.md.

@lexluo09
Copy link
Contributor Author

lexluo09 commented Aug 29, 2024 •
edited
Loading

And BTW, you should run mvn package -Dgenerate-config-docs -pl flink-docs -am -nsu -DskipTests -Pskip-webui-build if you add/change the configurations. Please check the instructions in flink-docs/README.md.

@Zakelly Thank you for your suggestion. I have executed the command to generate the documentation for the configuration as per your recommendation.

@Zakelly
Copy link
Contributor

Zakelly commented Aug 30, 2024

The CI failed. Would you mind fixing this?

@lexluo09
Copy link
Contributor Author

lexluo09 commented Aug 30, 2024 •
edited
Loading

The CI failed. Would you mind fixing this?

ok

@lexluo09
Copy link
Contributor Author

@flinkbot run azure

@lexluo09 lexluo09 closed this Aug 30, 2024
@lexluo09 lexluo09 reopened this Aug 30, 2024
@lexluo09
Copy link
Contributor Author

@flinkbot run azure

Zakelly
Zakelly reviewed Sep 2, 2024
View reviewed changes
Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lexluo09 Sorry for the late reply, overall looks good. Only one minor thing:

flink-core/src/main/java/org/apache/flink/api/common/state/StateTtlConfig.java Show resolved Hide resolved
@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 2, 2024

@lexluo09 Sorry for the late reply, overall looks good. Only one minor thing:

Thank you for taking the time to help review the code. It has been fixed now.

@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 2, 2024

@flinkbot run azure

1 similar comment
@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 2, 2024

@flinkbot run azure

Zakelly
Zakelly reviewed Sep 2, 2024
View reviewed changes
Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

flink-core/src/main/java/org/apache/flink/api/common/state/StateTtlConfig.java Outdated Show resolved Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/state/StateTtlConfig.java Outdated Show resolved Hide resolved
@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 2, 2024

How about this?

Very professional, thank you for your suggestions.

@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 2, 2024

image image I need help confirming a question: In ProtoUtils, the builder.cleanupInRocksdbCompactFilter only sets queryTimeAfterNumEntries and does not set periodicCompactionTime. Should we handle this situation for compatibility?

@Zakelly

@Zakelly
Copy link
Contributor

Zakelly commented Sep 2, 2024

I need help confirming a question: In ProtoUtils, the builder.cleanupInRocksdbCompactFilter only sets queryTimeAfterNumEntries and does not set periodicCompactionTime. Should we handle this situation for compatibility?
@Zakelly

@lexluo09
Yes, we should keep compatibility for that. I think in current PR, if it does builder.cleanupInRocksdbCompactFilter(rocksdbCompactFilterCleanupStrategyProto.getQueryTimeAfterNumEntries());, the DEFAULT_PERIODIC_COMPACTION_TIME still takes effect, aka 30 days set. So no compatibility issues, right?

@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 2, 2024

Yes, we should keep compatibility for that. I think in current PR, if it does builder.cleanupInRocksdbCompactFilter(rocksdbCompactFilterCleanupStrategyProto.getQueryTimeAfterNumEntries());, the DEFAULT_PERIODIC_COMPACTION_TIME still takes effect, aka 30 days set. So no compatibility issues, right?

Yes, I also think this is reasonable. The DEFAULT_PERIODIC_COMPACTION_TIME will still be retained.

@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 2, 2024

@flinkbot run azure

@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 2, 2024

@Zakelly Thank you for taking the time to review. I have made all the necessary changes. When you have the time, please kindly take another look. Thank you for your assistance.

@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 2, 2024

@flinkbot run azure

@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 3, 2024

@Zakelly The CI tests have passed. When you have time, could you please take a look? Thank you for your assistance.

Zakelly
Zakelly reviewed Sep 3, 2024
View reviewed changes
Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not important things as below:

And @fredia would please do a double check on this?

flink-core/src/main/java/org/apache/flink/api/common/state/StateTtlConfig.java Outdated Show resolved Hide resolved
flink-core/src/main/java/org/apache/flink/api/common/state/StateTtlConfig.java Outdated Show resolved Hide resolved
@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 3, 2024

@flinkbot run azure

Zakelly
Zakelly approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Zakelly
Copy link
Contributor

Zakelly commented Sep 4, 2024

@flinkbot run azure

@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 4, 2024

LGTM

Thank you for helping with the review.

@lexluo09
Copy link
Contributor Author

lexluo09 commented Sep 4, 2024

@flinkbot run azure

@Zakelly Zakelly merged commit a13b7ea into apache:master Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lincoln-lil lincoln-lil lincoln-lil left review comments

@Zakelly Zakelly Zakelly approved these changes

Assignees
No one assigned
Labels
component=<none>
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lexluo09 @flinkbot @lincoln-lil @Zakelly