-
Notifications
You must be signed in to change notification settings - Fork 970
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
Cleaned up BucketApplicator::Counters #3566
Conversation
src/bucket/BucketApplicator.h
Outdated
@@ -32,34 +32,26 @@ class BucketApplicator | |||
public: | |||
class Counters | |||
{ | |||
// indexed by LedgerEntryType, i.e. offer counter == CounterArray[OFFER] | |||
// array size == num enums in LedgerEntryType | |||
typedef std::array<uint64_t, 8> CounterArray; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as this is not performance critical, you should just make this a map LedgerEntryType -> uint64_t
, that way you don't need to hardcode anything
src/bucket/BucketApplicator.cpp
Outdated
case CONFIG_SETTING: | ||
++mConfigSettingUpsert; | ||
break; | ||
#ifndef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should not need those ifdefs anymore (see my comment on using maps)
src/bucket/BucketApplicator.cpp
Outdated
CounterArray const& upserted, | ||
CounterArray const& deleted) | ||
{ | ||
return fmt::format("for {}-entry bucket {}.{} au:{} ad:{} tu:{} td:{} " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to preserve the exact output, so you should just use iterators.
This will also fix the bug that we're not logging properly if ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
is set
@SirTyson any reason not to rebase and merge this? Looks like a useful change. |
f91e28c
to
0bf2e31
Compare
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question, otherwise looks good. Please squash the commits and we can merge this.
++mTTLDelete; | ||
break; | ||
} | ||
auto let = e.deadEntry().type(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should preserve the invariant regarding deletion of CONFIG_SETTING (it's illegal, so we should crash)
src/bucket/BucketApplicator.h
Outdated
@@ -32,39 +32,24 @@ class BucketApplicator | |||
public: | |||
class Counters | |||
{ | |||
// LedgerEntryType -> num_upserted, num_deleted | |||
typedef std::map<LedgerEntryType, std::pair<uint64_t, uint64_t>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turning std::pair into a struct would help with readability.
0bf2e31
to
b4777b3
Compare
r+ b4777b3 |
Description
Resolves #3114. Cleans up
BucketApplicator::Counters
.Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)