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

[testclient] Printing aggregated data when client exit #11985

Merged
merged 2 commits into from
Sep 20, 2021
Merged

[testclient] Printing aggregated data when client exit #11985

merged 2 commits into from
Sep 20, 2021

Conversation

yuruguo
Copy link
Contributor

@yuruguo yuruguo commented Sep 9, 2021

Motivation

Printing aggregated data includes throughput and delay when the client exits in PerformanceClient / ManagedLedgerWriter

Modifications

  • Add printAggregatedStats method to print delay data in class PerformanceClient
  • Add a ShutdownHook that call printAggregatedThroughput and printAggregatedStats in class PerformanceClient
  • Add printAggregatedThroughput into ShutdownHook in class ManagedLedgerWriter

Documentation

  • no-need-doc

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Sep 10, 2021
@@ -45,6 +45,7 @@
@WebSocket(maxTextMessageSize = 64 * 1024)
public class SimpleTestProducerSocket {
public static Recorder recorder = new Recorder(TimeUnit.SECONDS.toMillis(120000), 5);
public static Recorder cumulativeRecorder = new Recorder(TimeUnit.SECONDS.toMillis(120000), 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why need to add a new field cumulativeRecorder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistent with other implementations such as PerformanceProducer, PerformanceReader, ManagedLedgerWriter and PerformanceConsumer

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn’t seem to be used in SimpleTestProducerSocket

Copy link
Contributor Author

@yuruguo yuruguo Sep 14, 2021

Choose a reason for hiding this comment

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

I have removed this variable cumulativeRecorder in SimpleTestProducerSocket and directly reuse recorder to get the aggregated data.

Copy link
Contributor

Choose a reason for hiding this comment

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

@congbobo184 @gaoran10 Could you take a look again?

@yuruguo yuruguo changed the title [testclient] Call printAggregatedStats method before client exit [testclient] Print aggregated data when client exit Sep 15, 2021
@yuruguo yuruguo changed the title [testclient] Print aggregated data when client exit [testclient] Printing aggregated data when client exit Sep 15, 2021
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

@BewareMyPower BewareMyPower added this to the 2.9.0 milestone Sep 16, 2021
@eolivelli eolivelli merged commit 3c770a1 into apache:master Sep 20, 2021
codelipenghui pushed a commit that referenced this pull request Sep 29, 2021
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Sep 29, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tool cherry-picked/branch-2.8 Archived: 2.8 is end of life doc-not-needed Your PR changes do not impact docs release/2.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants