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.

Sign up for GitHub

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

Jump to bottom

Fix ReadEntryProcessor v2 SchedulingDelayStats #3758

Merged
merged 3 commits into from
Feb 27, 2023
Merged

Fix ReadEntryProcessor v2 SchedulingDelayStats #3758

merged 3 commits into from
Feb 27, 2023

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Jan 31, 2023

Descriptions of the changes in this PR:

Motivation

We registered ReadEntrySchedulingDelayStats of ReadEntryProcessor as WriteThreadQueuedLatency mistakenly, so we need fix it.

Changes

Register ReadEntrySchedulingDelayStats if ReadEntryProcessor

@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2023 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (master@c7cc668). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3758   +/-   ##
=========================================
  Coverage          ?   47.36%           
  Complexity        ?     4501           
=========================================
  Files             ?      468           
  Lines             ?    40840           
  Branches          ?     5236           
=========================================
  Hits              ?    19343           
  Misses            ?    19501           
  Partials          ?     1996
Flag Coverage Δ
remaining 29.31% <0.00%> (?)
replication 41.38% <0.00%> (?)
tls 21.09% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

hangc0276
hangc0276 reviewed Jan 31, 2023
View reviewed changes
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java Outdated
@@ -166,8 +166,14 @@ protected void sendResponseAndWait(int rc, Object response, OpStatsLogger statsL

@Override
public void run() {
requestProcessor.getRequestStats().getWriteThreadQueuedLatency()
.registerSuccessfulEvent(MathUtils.elapsedNanos(enqueueNanos), TimeUnit.NANOSECONDS);
if (this instanceof ReadEntryProcessor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use request instanceof BookieProtocol.ReadRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. It makes senses to me. This could uniform the code style. Updated @hangc0276

@hangc0276
Copy link
Contributor

@lordcheng10 Please help take a look at this Pr, thanks.

lordcheng10
lordcheng10 approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor

@lordcheng10 lordcheng10 left a comment

Choose a reason for hiding this comment

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

LGTM

@lordcheng10
Copy link
Contributor

lordcheng10 commented Feb 2, 2023 •
edited

@hangc0276 Should the following code also use ParsedAddRequest to judge? for example:
if (request instanceof BookieProtocol. ParsedAddRequest) {
requestProcessor.onAddRequestFinish();
}

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java

Lines 178 to 180 in accaa69

if (request instanceof BookieProtocol.AddRequest) {
requestProcessor.onAddRequestFinish();
}

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Feb 2, 2023 •
edited

@hangc0276 Should the following code also use ParsedAddRequest to judge? for example: if (request instanceof BookieProtocol. ParsedAddRequest) { requestProcessor.onAddRequestFinish(); }

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java

Lines 178 to 180 in accaa69

if (request instanceof BookieProtocol.AddRequest) {
requestProcessor.onAddRequestFinish();
}

I think it looks like a bug that we shoud use ParsedAddRequest. We could fix it in another PR and add a test to verify it? WDYT @hangc0276 @lordcheng10 .

Try to fix this bug here #3761

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Feb 3, 2023

@StevenLuMT @hangc0276 PTAL

@AnonHxy
Copy link
Contributor Author

AnonHxy commented Feb 14, 2023

@hangc0276 PTAL

StevenLuMT
StevenLuMT approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

LGTM

@hangc0276 hangc0276 added this to the 4.16.0 milestone Feb 27, 2023
@hangc0276 hangc0276 merged commit 08ef649 into apache:master Feb 27, 2023
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Jun 26, 2023
@AnonHxy @hangc0276
Fix ReadEntryProcessor v2 SchedulingDelayStats (apache#3758)
d23e63c 
Descriptions of the changes in this PR:

We registered `ReadEntrySchedulingDelayStats` of `ReadEntryProcessor` as `WriteThreadQueuedLatency` mistakenly, so we need fix it.

Register `ReadEntrySchedulingDelayStats`  if `ReadEntryProcessor`

(cherry picked from commit 08ef649)
zymap pushed a commit that referenced this pull request Dec 6, 2023
@AnonHxy @zymap
Fix ReadEntryProcessor v2 SchedulingDelayStats (#3758)
2eb1c36 
Descriptions of the changes in this PR:

### Motivation

We registered `ReadEntrySchedulingDelayStats` of `ReadEntryProcessor` as `WriteThreadQueuedLatency` mistakenly, so we need fix it.

### Changes

Register `ReadEntrySchedulingDelayStats`  if `ReadEntryProcessor`

(cherry picked from commit 08ef649)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangc0276 hangc0276 hangc0276 approved these changes

@lordcheng10 lordcheng10 lordcheng10 approved these changes

@StevenLuMT StevenLuMT StevenLuMT approved these changes

Assignees

@AnonHxy AnonHxy

Labels
area/stats cherry-picked/branch-4.14 cherry-picked/branch-4.15 release/4.14.8 release/4.15.5
Projects
None yet
Milestone
4.16.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@AnonHxy @codecov-commenter @hangc0276 @lordcheng10 @StevenLuMT @zymap