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.

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-34386][state] Add RocksDB bloom filter metrics #24274

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

hejufang
Copy link

@hejufang hejufang commented Feb 6, 2024

What is the purpose of the change

Add RocksDB bloom filter metrics.

Brief change log

Get RocksDB bloom filter metrics via RocksDB Statistics, and report it via Metrics reporter.

Verifying this change

Please make sure both new and modified tests in this PR follows the conventions defined in our code quality guide: https://flink.apache.org/contributing/code-style-and-quality-common.html#testing

This change is a trivial rework / code cleanup without any test coverage.

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? yes
  • If yes, how is the feature documented? not documented

@flinkbot
Copy link
Collaborator

flinkbot commented Feb 6, 2024

CI report:

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

Copy link
Contributor

@pnowojski pnowojski left a comment

Choose a reason for hiding this comment

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

LGTM, but you forgot to regenerate the docs, hence the test failure. Could you fix that?

@hejufang
Copy link
Author

hejufang commented Feb 7, 2024

@flinkbot run azure

@hejufang hejufang force-pushed the FLINK-34386 branch 2 times, most recently from 7cc4dcb to b9345a7 Compare February 8, 2024 06:06
@pnowojski
Copy link
Contributor

@flinkbot run azure

@hejufang
Copy link
Author

hejufang commented Feb 9, 2024

@flinkbot run azure

@JingGe
Copy link
Contributor

JingGe commented Feb 9, 2024

There are some issue while building the hadoop image: https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=57409&view=logs&j=ef799394-2d67-5ff4-b2e5-410b80c9c0af&t=9e5768bc-daae-5f5f-1861-e58617922c7a&l=10543

ERROR: failed to solve: process "/bin/sh -c set -x && mkdir -p /usr/local/hadoop && tar -xf /tmp/hadoop.tar.gz --strip-components=1 -C /usr/local/hadoop && rm /tmp/hadoop.tar.gz*" did not complete successfully: exit code: 2
Service 'master' failed to build : Build failed
Feb 09 07:20:05 Command: build_image failed. Retrying...
Feb 09 07:20:07 Command: build_image failed 5 times.
Feb 09 07:20:07 ERROR: Could not build hadoop image. Aborting...

@JingGe
Copy link
Contributor

JingGe commented Feb 13, 2024

@hejufang the issue should be fixed if you rebase your branch

@hejufang hejufang force-pushed the FLINK-34386 branch 2 times, most recently from 0948c38 to 3b193ff Compare February 17, 2024 06:45
@hejufang
Copy link
Author

@flinkbot run azure

@hejufang
Copy link
Author

@JingGe Thank you for your advice. The issue is fixed.

@hejufang
Copy link
Author

@pnowojski kindly remind.

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.

Thanks for the PR! It looks good.

.booleanType()
.defaultValue(false)
.withDescription(
"Monitor the total count of bloom FullFilter has not avoided the reads and data actually exist.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest a refine, such as "Monitor the count of reads avoided by full filter while the data actually exists in RocksDB".

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your advice, I’ve refined this description.

@hejufang
Copy link
Author

@flinkbot run azure

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. Thanks for the update.

@pnowojski pnowojski merged commit 890a995 into apache:master Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants