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

feat: dynamic flow control p3: add FlowControllerEventStats #1332

Merged
merged 14 commits into from Apr 5, 2021

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Mar 22, 2021

Implementation of go/veneer-dynamic-flow-control part 3, adding a class to record flow control events

@mutianf mutianf requested review from as code owners Mar 22, 2021
@google-cla google-cla bot added the cla: yes label Mar 22, 2021
@codecov
Copy link

@codecov codecov bot commented Mar 22, 2021

Codecov Report

Merging #1332 (522be6c) into master (51c40ab) will increase coverage by 0.14%.
The diff coverage is 92.68%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1332      +/-   ##
============================================
+ Coverage     81.31%   81.45%   +0.14%     
- Complexity     1338     1347       +9     
============================================
  Files           210      211       +1     
  Lines          5690     5728      +38     
  Branches        521      526       +5     
============================================
+ Hits           4627     4666      +39     
+ Misses          852      850       -2     
- Partials        211      212       +1     
Impacted Files Coverage Δ Complexity Δ
.../java/com/google/api/gax/batching/BatcherImpl.java 96.06% <ø> (+1.12%) 23.00 <0.00> (+1.00)
...google/api/gax/batching/FlowControlEventStats.java 88.00% <88.00%> (ø) 5.00 <5.00> (?)
...va/com/google/api/gax/batching/FlowController.java 89.68% <100.00%> (+1.18%) 36.00 <1.00> (+2.00)
.../google/api/gax/batching/NonBlockingSemaphore.java 81.57% <0.00%> (+5.26%) 12.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51c40ab...522be6c. Read the comment docs.

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

lgtm with a nit.

@vam-google wanna take a pass?

Copy link
Contributor

@vam-google vam-google left a comment

LGTM with a bunch of minor comments.

return new AutoValue_FlowControlEventStats_FlowControlEvent(timestampMs, null, exception);
}

public abstract long getTimestampMs();
Copy link
Contributor

@vam-google vam-google Mar 31, 2021

Choose a reason for hiding this comment

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

Subjective, but it looks like this class does not need to be AutoValue. It is way less trivial than most of the AutoValue classes, which are usually just a set of setters/getters (not the case here). Please consider just storing the throttledtimeInMs, exception and timestampMs values manually in this class.

Copy link
Contributor

@elharo elharo Apr 1, 2021

Choose a reason for hiding this comment

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

agreed

Copy link
Collaborator

@igorbernstein2 igorbernstein2 Apr 1, 2021

Choose a reason for hiding this comment

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

AutoValue doesnt actually hurt anything here. I generally prefer AutoValue here to communicate the fact that this is a simple value type

return new AutoValue_FlowControlEventStats_FlowControlEvent(timestampMs, null, exception);
}

public abstract long getTimestampMs();
Copy link
Contributor

@elharo elharo Apr 1, 2021

Choose a reason for hiding this comment

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

agreed

@igorbernstein2 igorbernstein2 added kokoro:force-run automerge labels Apr 2, 2021
@gcf-merge-on-green
Copy link
Contributor

@gcf-merge-on-green gcf-merge-on-green bot commented Apr 2, 2021

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge label Apr 2, 2021
@mutianf mutianf force-pushed the dynamic_flow_control_p3 branch from 76fdf5d to 74eae2b Compare Apr 5, 2021
@igorbernstein2 igorbernstein2 added kokoro:force-run and removed kokoro:force-run labels Apr 5, 2021
@igorbernstein2 igorbernstein2 added the automerge label Apr 5, 2021
@igorbernstein2 igorbernstein2 merged commit 5329ea4 into googleapis:master Apr 5, 2021
9 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge label Apr 5, 2021
@mutianf mutianf deleted the dynamic_flow_control_p3 branch Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kokoro:force-run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants