BewareMyPower commented on code in PR #23958:
URL: https://github.com/apache/pulsar/pull/23958#discussion_r1950772046
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/PendingReadsManager.java:
##########
@@ -211,73 +211,95 @@ private FindPendingReadOutcome
findPendingRead(PendingReadKey key, ConcurrentMap
private class PendingRead {
final PendingReadKey key;
final ConcurrentMap<PendingReadKey, PendingRead> ledgerCache;
- final List<ReadEntriesCallbackWithContext> callbacks = new
ArrayList<>(1);
- boolean completed = false;
+ final List<ReadEntriesCallbackWithContext> listeners = new
ArrayList<>(1);
+ PendingReadState state = PendingReadState.INITIALISED;
+
+ enum PendingReadState {
+ INITIALISED,
+ ATTACHED,
+ COMPLETED
+ }
public PendingRead(PendingReadKey key,
ConcurrentMap<PendingReadKey, PendingRead>
ledgerCache) {
this.key = key;
this.ledgerCache = ledgerCache;
}
- public void attach(CompletableFuture<List<EntryImpl>> handle) {
+ public synchronized void attach(CompletableFuture<List<EntryImpl>>
handle) {
+ if (state != PendingReadState.INITIALISED) {
+ // this shouldn't ever happen. this is here to prevent misuse
in future changes
+ throw new IllegalStateException("Unexpected state " + state +
" for PendingRead for key " + key);
+ }
+ state = PendingReadState.ATTACHED;
Review Comment:
Yes, to resolve the deadlock issue you only need to avoid acquiring the lock
when executing the callback because the callback might call another method that
acquires the lock.
My comment here is just to improve the code. Code with synchronized
everywhere could affect the code readability in some cases. With more fields
that need synchronization, the code will be harder to maintain. When reading
the code, it burdens if it needs to know why a `synchronized` is here.
Eventually, it might make `synchronized` everywhere because the coder could
given up thinking why a lock is here.
`ManagedLedgerImpl` is exactly such an example.
----
Back to the code here, it's a typical case that an atomic variable should be
used.
```java
void runOnlyOnce() {
if (flag.compareAndSet(init, ready)) {
run(); // it guarantees
}
}
```
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/PendingReadsManager.java:
##########
@@ -211,73 +211,95 @@ private FindPendingReadOutcome
findPendingRead(PendingReadKey key, ConcurrentMap
private class PendingRead {
final PendingReadKey key;
final ConcurrentMap<PendingReadKey, PendingRead> ledgerCache;
- final List<ReadEntriesCallbackWithContext> callbacks = new
ArrayList<>(1);
- boolean completed = false;
+ final List<ReadEntriesCallbackWithContext> listeners = new
ArrayList<>(1);
+ PendingReadState state = PendingReadState.INITIALISED;
+
+ enum PendingReadState {
+ INITIALISED,
+ ATTACHED,
+ COMPLETED
+ }
public PendingRead(PendingReadKey key,
ConcurrentMap<PendingReadKey, PendingRead>
ledgerCache) {
this.key = key;
this.ledgerCache = ledgerCache;
}
- public void attach(CompletableFuture<List<EntryImpl>> handle) {
+ public synchronized void attach(CompletableFuture<List<EntryImpl>>
handle) {
+ if (state != PendingReadState.INITIALISED) {
+ // this shouldn't ever happen. this is here to prevent misuse
in future changes
+ throw new IllegalStateException("Unexpected state " + state +
" for PendingRead for key " + key);
+ }
+ state = PendingReadState.ATTACHED;
Review Comment:
Yes, to resolve the deadlock issue you only need to avoid acquiring the lock
when executing the callback because the callback might call another method that
acquires the lock.
My comment here is just to improve the code. Code with synchronized
everywhere could affect the code readability in some cases. With more fields
that need synchronization, the code will be harder to maintain. When reading
the code, it burdens if it needs to know why a `synchronized` is here.
Eventually, it might make `synchronized` everywhere because the coder could
given up thinking why a lock is here.
`ManagedLedgerImpl` is exactly such an example.
----
Back to the code here, it's a typical case that an atomic variable should be
used.
```java
void runOnlyOnce() {
if (flag.compareAndSet(init, ready)) {
run(); // it guarantees
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
