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

StallConditionType2String transfer rocksdb::WriteStallCondition may be error #2633

Closed
1 of 2 tasks
greatsharp opened this issue Nov 1, 2024 · 2 comments
Closed
1 of 2 tasks
Labels
bug type bug

Comments

@greatsharp
Copy link

greatsharp commented Nov 1, 2024

Search before asking

  • I had searched in the issues and found no similar issues.

Version

kvrocks version 2.10.0

Minimal reproduce step

In rocksdb, WriteStallCondition is declared below

enum class WriteStallCondition {
  kDelayed,
  kStopped,
  // Always add new WriteStallCondition before `kNormal`
  kNormal,
};

see https://github.com/facebook/rocksdb/blob/main/include/rocksdb/types.h

but in kvrocks /src/storage/event_listener.cc, function define

std::string StallConditionType2String(const rocksdb::WriteStallCondition type) {
  std::vector<std::string> stall_condition_strings = {"normal", "delay", "stop"};
  if (static_cast<size_t>(type) < stall_condition_strings.size()) {
    return stall_condition_strings[static_cast<size_t>(type)];
  }
  return "unknown";
}

if we pass type as WriteStallCondition::kNormal, then function StallConditionType2String returned 'stop';

totally reversed.

the EventListener::OnStallConditionsChanged will record a confused warning message.

What did you expect to see?

WriteStallCondition::kNormal should be transfer to normal,
kDelayed should delay,
kStopped should be stop

What did you see instead?

write stall condition was changed, from normal to stop

Anything Else?

No response

Are you willing to submit a PR?

  • I'm willing to submit a PR!
@greatsharp greatsharp added the bug type bug label Nov 1, 2024
@PragmaTwice
Copy link
Member

Good catch! Would you like to fix it?

std::vector<std::string> seems not a good way to do that since it can not guarantee to be compatible to new versions of rocksdb when a new enum value is introduced.

We can impl by a std::map<SomeRocksdbEnumType, std::string>.

@mapleFU
Copy link
Member

mapleFU commented Nov 1, 2024

What about https://github.com/facebook/rocksdb/blob/a28cc4a38c2c7693a4786083d317bfc2cf874d76/db/write_stall_stats.h#L18 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug type bug
Projects
None yet
Development

No branches or pull requests

3 participants