statistics.cc: fix mistype #11509
statistics.cc: fix mistype #11509
Conversation
|
Hi @loskutov! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@loskutov thanks for the PR to help make RocksDB better. As for the typo, we have some concerns that such change could cause breakage for users who are looking for exactly "errro" in their monitoring systems. We plan to do this type of clean up in a major release. Before that, we can give users enough heads up and offer a transitioning period by exposing the same counter with both the misspelled and the correct name, and clean up the misspelled name in a major release. Let me know if you are interested in adding the correctly named counters. Otherwise, we will create some internal task for this. Thanks! |
|
@jowlyzhang I've updated the PR, now the misspelled names are also available. |
monitoring/statistics_impl.h
Outdated
| @@ -131,6 +131,12 @@ inline void RecordTick(Statistics* statistics, uint32_t ticker_type, | |||
| uint64_t count = 1) { | |||
| if (statistics) { | |||
| statistics->recordTick(ticker_type, count); | |||
| if (ticker_type == ERROR_HANDLER_BG_ERROR_COUNT || | |||
There was a problem hiding this comment.
When we clean up the "_MISSPELLED" tickers, it's not very obvious this code needs to be taken care of too. Without that, it will cause counting to unintended tickers. I know it's more duplicated code, but what about adding the ticker incrementing code to the source of when these tickers's counter parts get incremented in error_handler.cc. That way, the clean up is purely removing a couple of lines.
BTW, do you mind adding an entry for this change to unreleased_history/public_api_changes directory, you basically need to create a separate .md file, e.g. fix_ticker_typo.md into the directory, and describe the change in the file.
| ERROR_HANDLER_BG_ERROR_COUNT, | ||
| ERROR_HANDLER_BG_ERROR_COUNT_MISSPELLED, |
There was a problem hiding this comment.
nit: would you mind adding this note as a such as TODO : deprecate the misspelled tickers in next major release.
@loskutov thanks for making the change. I added some minor comments! |
| @@ -0,0 +1,4 @@ | |||
| Some tickers (namely, `ERROR_HANDLER_BG_ERROR_COUNT`, `ERROR_HANDLER_BG_IO_ERROR_COUNT`, and `ERROR_HANDLER_BG_RETRYABLE_IO_ERROR_COUNT`) | |||
There was a problem hiding this comment.
nit: it would be great if this can be updated to refer to the publicly visible names of these tickers like rocksdb.error.handler.bg.errro.count, since ERROR_HANDLER_BG_ERROR_COUNT is the enum type RocksDB internally uses to refer to the ticker type.
How about:
Add new tickers: rocksdb.error.handler.bg.error.count, rocksdb.error.handler.bg.retryable.io.error.count, rocksdb.error.handler.bg.error.count to replace the misspelled ones: rocksdb.error.handler.bg.errro.count, rocksdb.error.handler.bg.retryable.io.errro.count, rocksdb.error.handler.bg.errro.count. Users should switch to use the new tickers before 9.0 release as the misspelled old tickers will be completely removed then.
We usually make this a one paragraph message so it shows up as one entry in HISTORY.md later when we make the release.
There was a problem hiding this comment.
@loskutov Thanks for the change. LGTM, just have some minor comment! Would you mind updating the PR with a short description, thanks!
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jowlyzhang merged this pull request in 7c67aee. |
Summary:
Add new tickers: `rocksdb.error.handler.bg.error.count`, `rocksdb.error.handler.bg.io.error.count`, `rocksdb.error.handler.bg.retryable.io.error.count` to replace the misspelled ones: `rocksdb.error.handler.bg.errro.count`, `rocksdb.error.handler.bg.io.errro.count`, `rocksdb.error.handler.bg.retryable.io.errro.count` ('error' instead of 'errro'). Users should switch to use the new tickers before 9.0 release as the misspelled old tickers will be completely removed then.
Pull Request resolved: facebook#11509
Reviewed By: ltamasi
Differential Revision: D46542809
Pulled By: jowlyzhang
fbshipit-source-id: a2a6d8354af46a060de81d40ef6f5336a80bd32e
Summary: As titled, the replacement tickers have been introduced in #11509 and in use since release 8.4. This PR completely removes the misspelled ones. Pull Request resolved: #12302 Test Plan: CI tests Reviewed By: jaykorean Differential Revision: D53196935 Pulled By: jowlyzhang fbshipit-source-id: 9c9d0d321247690db5edfdc52b4fecb2f1218979
Add new tickers:
rocksdb.error.handler.bg.error.count,rocksdb.error.handler.bg.io.error.count,rocksdb.error.handler.bg.retryable.io.error.countto replace the misspelled ones:rocksdb.error.handler.bg.errro.count,rocksdb.error.handler.bg.io.errro.count,rocksdb.error.handler.bg.retryable.io.errro.count('error' instead of 'errro'). Users should switch to use the new tickers before 9.0 release as the misspelled old tickers will be completely removed then.