[improve][broker] Add the MessageExpirer interface to make code clear #20800
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
### Motivation When I reviewed apache#20597, the unrelated changes in `PersistentTopicsBase` are hard to read. The logic could be simplified to: ```java PersistentSubscription sub = null; PersistentReplicator repl = null; if (metSomeCondition()) { repl = /* ... */; if (repl == null) { /* ... */ return; } } else { sub = /* ... */; if (repl == null) { /* ... */ return; } } final PersistentSubscription finalSub = sub; final PersistentReplicator finalRepl = repl; future.thenAccept(__ -> { if (metSomeCondition()) { repl.expireMessages(/* ... */); } else { sub.expireMessages(/* ... */); } }); ``` The code above is such a mess. It adds two final variables because the lambda can only capture final variables. The `metSomeCondition` check is performed unnecessarily twice. The original code is more hard to read because the logic in `/* ... */` takes a few lines so that the two calls of `metSomeCondition()` are not near. From the code search I see all these classes implement two `expireMessages` methods that accept an integer or a position. - PersistentMessageExpiryMonitor - PersistentSubscription - PersistentReplicator - NonPersistentSubscription The code can be simplified to introduce a new interface. ### Modifications Introduce a `MessageExpirer` interface and change the class hierarchy to: ``` // [I] is interface, [C] is class [I] MessageExpirer [I] Subscription [C] PersistentSubscription [C] NonPersistentSubscription [C] PersistentReplicator [C] PersistentMessageExpiryMonitor ``` The method invocation can be simplified much as shown in this patch. P.S. Inserting such an interface in the type hierarchy does not even break the ABI compatibility, see https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html
BewareMyPower
requested review from
lhotari,
eolivelli,
codelipenghui,
tisonkun,
Demogorgon314,
coderzc and
mattisonchao
|
@BewareMyPower Please add the following content to your PR description and select a checkbox: |
github-actions
bot
added
doc-not-needed
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
tisonkun
approved these changes
Jul 21, 2023
Codecov Report
@@ Coverage Diff @@
## master #20800 +/- ##
============================================
+ Coverage 72.61% 73.13% +0.51%
+ Complexity 31966 3755 -28211
============================================
Files 1868 1868
Lines 139185 139170 -15
Branches 15315 15314 -1
============================================
+ Hits 101071 101776 +705
+ Misses 30052 29341 -711
+ Partials 8062 8053 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
lifepuzzlefun
approved these changes
Jul 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
When I reviewed #20597, the changes in
PersistentTopicsBaseare hard to read. The logic could be simplified to:The code above is bad. It adds two final variables because the lambda can only capture final variables. The
metSomeConditioncheck is performed unnecessarily twice. The original code is more hard to read because the logic in/* ... */takes a few lines so that the two calls ofmetSomeCondition()are not near.From the code search I see all these classes implement two
expireMessagesmethods that accept an integer or a position.The code can be simplified to introduce a new interface.
Modifications
Introduce a
MessageExpirerinterface and change the class hierarchy to:The method invocation can be simplified much as shown in this patch.
P.S. Inserting such an interface in the type hierarchy does not even break the ABI compatibility, see https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: