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] [broker] Add subscription prefix for internal reader #23044

Merged

Conversation

hangc0276
Copy link
Contributor

Fixes #xyz

Main Issue: #xyz

PIP: #xyz

Motivation

We have many system topics, such as

  • __change_events
  • __transaction_buffer_snapshot
  • __transaction_buffer_snapshot_indexes
  • __transaction_buffer_snapshot_segments
  • transaction_coordinator_assign
  • _transaction_log
  • __transaction_pending_ack

In Pulsar Broker, we create an internal reader to fetch messages from those system topics. Due to we do not specify the subscription prefix, the reader will generate a random subscription name for each reader.

In PIP-355, we introduced a broker-level metric named pulsar_broker_out_bytes_total, which separate the system subscription traffic bytes and user subscription traffic bytes. Due to the internal readers don't have a subscription prefix, we group the internal reader's traffic bytes into user subscription traffic.

Modifications

In this PR, we introduce a system subscription prefix named __system_reader and group the internal reader's traffic into system subscription traffic bytes in metric pulsar_broker_out_bytes_total.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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

If the box was checked, please highlight the changes

  • 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:

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jul 17, 2024
@hangc0276 hangc0276 self-assigned this Jul 17, 2024
@hangc0276 hangc0276 added area/metrics category/functionality Some functions are not working such as get errors release/3.3.1 and removed doc-required Your PR changes impact docs and you will update later. labels Jul 17, 2024
@hangc0276 hangc0276 added this to the 3.4.0 milestone Jul 17, 2024
@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jul 17, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 73.40%. Comparing base (bbc6224) to head (1fea1e3).
Report is 468 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23044      +/-   ##
============================================
- Coverage     73.57%   73.40%   -0.17%     
- Complexity    32624    33493     +869     
============================================
  Files          1877     1917      +40     
  Lines        139502   144067    +4565     
  Branches      15299    15741     +442     
============================================
+ Hits         102638   105752    +3114     
- Misses        28908    30190    +1282     
- Partials       7956     8125     +169     
Flag Coverage Δ
inttests 27.48% <50.00%> (+2.90%) ⬆️
systests 24.80% <37.50%> (+0.47%) ⬆️
unittests 72.46% <62.50%> (-0.39%) ⬇️

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

Files Coverage Δ
...sar/broker/service/persistent/PersistentTopic.java 79.21% <100.00%> (+0.75%) ⬆️
...roker/systopic/TopicPoliciesSystemTopicClient.java 72.50% <100.00%> (+0.34%) ⬆️
...ransactionBufferSnapshotBaseSystemTopicClient.java 76.62% <100.00%> (+0.30%) ⬆️
.../apache/pulsar/common/naming/SystemTopicNames.java 77.77% <ø> (ø)
...oker/service/nonpersistent/NonPersistentTopic.java 71.31% <0.00%> (+1.84%) ⬆️

... and 503 files with indirect coverage changes

Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

LGTM, do we need a PIP for this ?

@hangc0276
Copy link
Contributor Author

LGTM, do we need a PIP for this ?

@Technoboy- It doesn't change any interface or configurations, and I think we do not need a PIP

@hangc0276 hangc0276 force-pushed the chenhang/add_cursor_subscription_name branch from 03bf9cf to 1fea1e3 Compare July 25, 2024 01:31
@hangc0276 hangc0276 merged commit c7310e3 into apache:master Jul 25, 2024
51 checks passed
Technoboy- pushed a commit that referenced this pull request Jul 25, 2024
### Motivation
We have many system topics, such as

__change_events
__transaction_buffer_snapshot
__transaction_buffer_snapshot_indexes
__transaction_buffer_snapshot_segments
transaction_coordinator_assign
_transaction_log
__transaction_pending_ack
In Pulsar Broker, we create an internal reader to fetch messages from those system topics. Due to we do not specify the subscription prefix, the reader will generate a random subscription name for each reader.

In PIP-355, we introduced a broker-level metric named pulsar_broker_out_bytes_total, which separate the system subscription traffic bytes and user subscription traffic bytes. Due to the internal readers don't have a subscription prefix, we group the internal reader's traffic bytes into user subscription traffic.

### Modifications
In this PR, we introduce a system subscription prefix named __system_reader and group the internal reader's traffic into system subscription traffic bytes in metric pulsar_broker_out_bytes_total.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics category/functionality Some functions are not working such as get errors cherry-picked/branch-3.3 doc-required Your PR changes impact docs and you will update later. ready-to-test release/3.3.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants