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

Export GetCompactionReasonString/GetFlushReasonString by moving them into listener.h #11778

Closed

Conversation

git-hulk
Copy link
Contributor

@git-hulk git-hulk commented Aug 31, 2023

Currently, rocksdb users would use the event listener to catch the compaction/flush event and log them if any. But now the reason is an integer type instead of a human-readable string, so we would like to convert them into a human-readable string.

Copy link
Contributor

@mrambacher mrambacher left a comment

Choose a reason for hiding this comment

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

Rather than moving the code, why not just publish the API publicly? This would make it available to you with fewer changes and less dependency on the implementation and strings.

@git-hulk
Copy link
Contributor Author

Rather than moving the code, why not just publish the API publicly? This would make it available to you with fewer changes and less dependency on the implementation and strings.

Hi @mrambacher, thanks for your suggestion. Which file should I put those two functions? does it make sense to put them in listener.h?

@mrambacher
Copy link
Contributor

Rather than moving the code, why not just publish the API publicly? This would make it available to you with fewer changes and less dependency on the implementation and strings.

Hi @mrambacher, thanks for your suggestion. Which file should I put those two functions? does it make sense to put them in listener.h?

@git-hulk I would suggest the declaration be in listener.h, since that is where the enums are specified.

@git-hulk
Copy link
Contributor Author

Rather than moving the code, why not just publish the API publicly? This would make it available to you with fewer changes and less dependency on the implementation and strings.

Hi @mrambacher, thanks for your suggestion. Which file should I put those two functions? does it make sense to put them in listener.h?

@git-hulk I would suggest the declaration be in listener.h, since that is where the enums are specified.

Got it, thank you!

@git-hulk git-hulk force-pushed the export-reason-to-string-functions branch from d8cdcfc to 56e6537 Compare August 31, 2023 15:43
@git-hulk git-hulk requested a review from mrambacher August 31, 2023 15:43
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ajkr merged this pull request in 3f54b96.

facebook-github-bot pushed a commit to facebook/mysql-5.6 that referenced this pull request Sep 29, 2023
Summary:
`GetCompactionReasonString()` will be exposed in
RocksDB 8.7, which includes PR facebook/rocksdb#11778 that exposes it. So update its definition here to be based on RocksDB version.

Differential Revision: D49755853

fbshipit-source-id: 1ff11f6708768b0039f88835e44272f525798ba2
hermanlee pushed a commit to hermanlee/mysql-5.6 that referenced this pull request Oct 18, 2023
Summary:
`GetCompactionReasonString()` will be exposed in
RocksDB 8.7, which includes PR facebook/rocksdb#11778 that exposes it. So update its definition here to be based on RocksDB version.

Differential Revision: D49755853
rockeet pushed a commit to topling/toplingdb that referenced this pull request Dec 18, 2023
…into listener.h (#11778)

Summary:
Currently, rocksdb users would use the event listener to catch the compaction/flush event and log them if any. But now the reason is an integer type instead of a human-readable string, so we would like to convert them into a human-readable string.

Pull Request resolved: facebook/rocksdb#11778

Reviewed By: jaykorean

Differential Revision: D49012934

Pulled By: ajkr

fbshipit-source-id: a4935b95d70c1be02aec65da7bf1c98a8cf8b933
rockeet pushed a commit to topling/toplingdb that referenced this pull request Sep 1, 2024
…into listener.h (#11778)

Summary:
Currently, rocksdb users would use the event listener to catch the compaction/flush event and log them if any. But now the reason is an integer type instead of a human-readable string, so we would like to convert them into a human-readable string.

Pull Request resolved: facebook/rocksdb#11778

Reviewed By: jaykorean

Differential Revision: D49012934

Pulled By: ajkr

fbshipit-source-id: a4935b95d70c1be02aec65da7bf1c98a8cf8b933
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants