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

[improve][client]: add --streaming option to topics partitioned-stats-internal #23380

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dlg99
Copy link
Contributor

@dlg99 dlg99 commented Sep 30, 2024

Motivation

Streaming the stats output directly to the standard output without parsing the response in memory.

Modifications

added --streaming flag for topics stats-internal/partitioned-stats-internal commands to stream the stats.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added unit tests.

Does this pull request potentially affect one of the following parts:

added --streaming flag for topics stats-internal/partitioned-stats-internal commands

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: dlg99#21

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 30, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 52.17391% with 11 lines in your changes missing coverage. Please review.

Project coverage is 74.52%. Comparing base (bbc6224) to head (cd88109).
Report is 667 commits behind head on master.

Files with missing lines Patch % Lines
...pache/pulsar/client/admin/internal/TopicsImpl.java 0.00% 9 Missing ⚠️
...in/java/org/apache/pulsar/admin/cli/CmdTopics.java 85.71% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23380      +/-   ##
============================================
+ Coverage     73.57%   74.52%   +0.94%     
- Complexity    32624    34468    +1844     
============================================
  Files          1877     1934      +57     
  Lines        139502   145158    +5656     
  Branches      15299    15874     +575     
============================================
+ Hits         102638   108176    +5538     
+ Misses        28908    28679     -229     
- Partials       7956     8303     +347     
Flag Coverage Δ
inttests 27.71% <0.00%> (+3.13%) ⬆️
systests 24.53% <0.00%> (+0.20%) ⬆️
unittests 73.87% <52.17%> (+1.03%) ⬆️

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

Files with missing lines Coverage Δ
...in/java/org/apache/pulsar/client/admin/Topics.java 78.57% <ø> (+1.07%) ⬆️
...in/java/org/apache/pulsar/admin/cli/CmdTopics.java 80.01% <85.71%> (-1.16%) ⬇️
...pache/pulsar/client/admin/internal/TopicsImpl.java 82.97% <0.00%> (+0.96%) ⬆️

... and 600 files with indirect coverage changes

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Instead of an InputStream, a Reader would be more appropriate for the interface.

The reason for this is in this case, the payload is most likely UTF-8 encoded which uses multi-byte characters for most non-ascii characters.
When reading in 2048 chunks, a multi-byte character might get split at the boundary which will corrupt the output.

Using a Reader would address this without any additional logic.
A long time ago I worked on such a fix for Gradle, https://issues.gradle.org/browse/GRADLE-3329. In that case there was custom buffering and fixing the issue wasn't trivial. I ended up using java.nio.charset.CharsetDecoder and java.nio.charset.CoderResult#isUnderflow to detect when a byte is missing.

A Reader will buffer the underlying bytes when there's a multi-byte character case and the remaining byte is not yet readable. That's why it's better to rely on a Reader in the interface.

@lhotari
Copy link
Member

lhotari commented Oct 14, 2024

slightly related #18263

@lhotari
Copy link
Member

lhotari commented Oct 15, 2024

@dlg99 please address the review feedback, #23380 (review).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants