-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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][broker][PIP-195] Support internal cursor properties - part4 #17712
[feat][broker][PIP-195] Support internal cursor properties - part4 #17712
Conversation
1b66fbd
to
a010aa7
Compare
the tests failed in Broker Group 3 because of OOM, I filed #17714 . It's most likely not caused by the changes in this PR. |
a010aa7
to
5f60f6d
Compare
The pr had no activity for 30 days, mark with Stale label. |
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Outdated
Show resolved
Hide resolved
...ged-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorPropertiesTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears to have a minor compatibility issue - if the user has previously used a format like CURSOR_INTERNAL_PROPERTY_PREFIX
in cursor properties. I think it's a small probability event though.
Codecov Report
@@ Coverage Diff @@
## master #17712 +/- ##
============================================
+ Coverage 47.23% 47.91% +0.68%
+ Complexity 10430 9349 -1081
============================================
Files 692 613 -79
Lines 67766 58391 -9375
Branches 7260 6087 -1173
============================================
- Hits 32009 27978 -4031
+ Misses 32192 27379 -4813
+ Partials 3565 3034 -531
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Master Issue: #16763
Motivation
Now, the cursor properties support overall update and modifying a single value concurrently by #17164, but
setCursorProperties
will all key-value is removed before update properties, this will cause internal property loss, so use a special prefix to prevent internal properties.More context see: #17164 (comment), #17164 (comment), #17164 (comment)
Modifications
Use a special prefix to prevent internal properties.
Verifying this change
(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:)
Documentation
doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)
Matching PR in forked repository
PR in forked repository: coderzc#4