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

Fix bug to dedicated flusher #174

Merged
merged 3 commits into from
Sep 10, 2024
Merged

Fix bug to dedicated flusher #174

merged 3 commits into from
Sep 10, 2024

Conversation

ZexiLiu
Copy link
Contributor

@ZexiLiu ZexiLiu commented Sep 10, 2024

No description provided.

Copy link
Contributor

@greensky00 greensky00 left a comment

Choose a reason for hiding this comment

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

Need some cosmetic change. Let me take it over.

src/flusher.h Outdated
@@ -73,17 +73,20 @@ class FlusherQueue {

class Flusher : public WorkerBase {
public:
enum FlusherType { GENERIC = 0x0, ONLY_HANDLE_ASYNC = 0x1, ONLY_HANDLE_SYNC = 0x2 };
Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree on GENERIC for non-dedicated situation, but the other two need to be renamed. Actually both are async processing. I'd rather name them

enum FlusherType { GENERIC = 0x0, FLUSH_ON_DEMAND = 0x1, FLUSH_ON_CONDITION = 0x2 };

src/db_mgr.cc Outdated
Comment on lines 106 to 110
std::string t_name = flusher_type == Flusher::FlusherType::ONLY_HANDLE_ASYNC
? "flusher_async_" + std::to_string(ii)
: (flusher_type == Flusher::FlusherType::ONLY_HANDLE_SYNC
? "flusher_sync_" + std::to_string(ii)
: "flusher_generic_" + std::to_string(ii));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as below, I'd rather do flusher, flusher_od, flusher_oc. Also using switch will be better for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with flusher_od and flusher_oc, but should we do flusher_gen for GENERIC case? Because the worker invocation is prefix base. Though GENERIC and ON_DEMAND/ON_CONDITION is kind of mutual exclusive.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

@ZexiLiu ZexiLiu left a comment

Choose a reason for hiding this comment

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

LGTM

@greensky00 greensky00 merged commit fb1f0a7 into eBay:master Sep 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants