Skip to content

[improve][broker] Add javadoc for PulsarDecoder handle* method parameter lifecycle #18998

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
Merged
lhotari merged 1 commit into from
Dec 20, 2022
Merged

[improve][broker] Add javadoc for PulsarDecoder handle* method parameter lifecycle #18998

lhotari merged 1 commit into from
Dec 20, 2022

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Dec 20, 2022 •
edited
Loading

Motivation

The lifecycle of the PulsarDecoder handle* method parameters isn't currently documented.

This PR adds this kind of paragraph:

Please be aware that the decoded protocol command instance passed to a handle* method is cleared and reused for the next protocol command after the method completes. This is done in order to minimize object allocations for performance reasons. It is not allowed to retain a reference to the handle* method parameter command instance after the method returns. If you need to pass an instance of the command instance to another thread or retain a reference to it after the handle* method completes, you must make a deep copy of the command instance.

See #18987 or #10215 for a sample issues where a related problem was fixed.

Modifications

Add Javadoc to PulsarDecoder and reference it in the implementation classes.

Documentation

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

Matching PR in forked repository

PR in forked repository: lhotari#115

@lhotari lhotari added this to the 2.12.0 milestone Dec 20, 2022
@lhotari lhotari self-assigned this Dec 20, 2022
@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Dec 20, 2022
@lhotari lhotari mentioned this pull request Dec 20, 2022
Closed
@lhotari lhotari force-pushed the lh-add-javadoc-for-PulsarDecoder-handle-parameter-lifecycle branch from 52ec790 to ba09cc2 Compare December 20, 2022 09:29
@lhotari lhotari requested a review from cbornet December 20, 2022 09:30
@lhotari lhotari force-pushed the lh-add-javadoc-for-PulsarDecoder-handle-parameter-lifecycle branch from ba09cc2 to 36dbe20 Compare December 20, 2022 09:31
@lhotari lhotari force-pushed the lh-add-javadoc-for-PulsarDecoder-handle-parameter-lifecycle branch from 36dbe20 to b272661 Compare December 20, 2022 09:32
@lhotari lhotari mentioned this pull request Dec 20, 2022
Merged
1 task
@lhotari
Copy link
Member Author

lhotari commented Dec 20, 2022

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022 •
edited
Loading

Codecov Report

Merging #18998 (b272661) into master (feb3cb4) will increase coverage by 0.97%.
The diff coverage is 13.58%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18998      +/-   ##
============================================
+ Coverage     46.35%   47.32%   +0.97%     
- Complexity     8939     9414     +475     
============================================
  Files           597      626      +29     
  Lines         56858    59237    +2379     
  Branches       5905     6153     +248     
============================================
+ Hits          26357    28036    +1679     
- Misses        27616    28159     +543     
- Partials       2885     3042     +157
Flag Coverage Δ
unittests 47.32% <13.58%> (+0.97%) ⬆️

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

Impacted Files Coverage Δ
...elayed/bucket/BookkeeperBucketSnapshotStorage.java 0.00% <0.00%> (ø)
...rg/apache/pulsar/broker/delayed/bucket/Bucket.java 0.00% <0.00%> (ø)
...yed/bucket/BucketSnapshotPersistenceException.java 0.00% <0.00%> (ø)
...d/bucket/BucketSnapshotSerializationException.java 0.00% <0.00%> (ø)
.../pulsar/broker/service/BrokerServiceException.java 47.27% <0.00%> (+0.97%) ⬆️
.../java/org/apache/pulsar/client/impl/ClientCnx.java 29.97% <ø> (ø)
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 15.09% <0.00%> (+<0.01%) ⬆️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 22.80% <0.00%> (+0.01%) ⬆️
...ache/pulsar/client/impl/ZeroQueueConsumerImpl.java 0.00% <0.00%> (ø)
...org/apache/pulsar/proxy/server/ProxyClientCnx.java 46.51% <ø> (ø)
... and 85 more

michaeljmarshall
michaeljmarshall approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari merged commit f360807 into apache:master Dec 20, 2022
@lhotari lhotari mentioned this pull request Oct 9, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@eolivelli eolivelli Awaiting requested review from eolivelli

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@nodece nodece Awaiting requested review from nodece

@tisonkun tisonkun Awaiting requested review from tisonkun

@BewareMyPower BewareMyPower Awaiting requested review from BewareMyPower

@cbornet cbornet Awaiting requested review from cbornet

@congbobo184 congbobo184 Awaiting requested review from congbobo184

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

@merlimat merlimat Awaiting requested review from merlimat

Assignees

@lhotari lhotari

Labels

area/broker doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. ready-to-test

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@lhotari @codecov-commenter @nicoloboschi @michaeljmarshall