Copilot commented on code in PR #4701:
URL: https://github.com/apache/bookkeeper/pull/4701#discussion_r2693668080
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java:
##########
@@ -53,16 +53,18 @@ class HandleFactoryImpl implements HandleFactory,
LedgerDeletionListener {
@Override
public LedgerDescriptor getHandle(final long ledgerId, final byte[]
masterKey, boolean journalReplay)
throws IOException, BookieException {
- LedgerDescriptor handle = ledgers.get(ledgerId);
-
- if (handle == null) {
- if (!journalReplay &&
recentlyFencedAndDeletedLedgers.getIfPresent(ledgerId) != null) {
- throw
BookieException.create(BookieException.Code.LedgerFencedAndDeletedException);
+ if (!ledgers.containsKey(ledgerId)) {
+ synchronized (ledgers) {
+ if (!ledgers.containsKey(ledgerId)) {
+ if (!journalReplay &&
recentlyFencedAndDeletedLedgers.getIfPresent(ledgerId) != null) {
+ throw
BookieException.create(BookieException.Code.LedgerFencedAndDeletedException);
+ }
+ LedgerDescriptor handle =
LedgerDescriptor.create(masterKey, ledgerId, ledgerStorage);
+ ledgers.putIfAbsent(ledgerId, handle);
Review Comment:
The `putIfAbsent` on line 63 is unnecessary and potentially wasteful. Since
we're already inside a synchronized block with a double-check, the key is
guaranteed not to exist. Use `put()` instead of `putIfAbsent()` to avoid the
redundant check and potential creation of unused LedgerDescriptor instances.
```suggestion
ledgers.put(ledgerId, handle);
```
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java:
##########
@@ -53,16 +53,18 @@ class HandleFactoryImpl implements HandleFactory,
LedgerDeletionListener {
@Override
public LedgerDescriptor getHandle(final long ledgerId, final byte[]
masterKey, boolean journalReplay)
throws IOException, BookieException {
- LedgerDescriptor handle = ledgers.get(ledgerId);
-
- if (handle == null) {
- if (!journalReplay &&
recentlyFencedAndDeletedLedgers.getIfPresent(ledgerId) != null) {
- throw
BookieException.create(BookieException.Code.LedgerFencedAndDeletedException);
+ if (!ledgers.containsKey(ledgerId)) {
+ synchronized (ledgers) {
+ if (!ledgers.containsKey(ledgerId)) {
+ if (!journalReplay &&
recentlyFencedAndDeletedLedgers.getIfPresent(ledgerId) != null) {
+ throw
BookieException.create(BookieException.Code.LedgerFencedAndDeletedException);
+ }
+ LedgerDescriptor handle =
LedgerDescriptor.create(masterKey, ledgerId, ledgerStorage);
+ ledgers.putIfAbsent(ledgerId, handle);
+ }
}
- handle = LedgerDescriptor.create(masterKey, ledgerId,
ledgerStorage);
- ledgers.putIfAbsent(ledgerId, handle);
}
-
+ LedgerDescriptor handle = ledgers.get(ledgerId);
Review Comment:
Potential null pointer exception: After the synchronized block exits,
another thread could remove the ledger (via `ledgerDeleted`) before line 67
executes, causing `handle` to be null and resulting in a NullPointerException
at line 68. Add a null check after line 67.
```suggestion
LedgerDescriptor handle = ledgers.get(ledgerId);
if (handle == null) {
synchronized (ledgers) {
handle = ledgers.get(ledgerId);
if (handle == null) {
if (!journalReplay &&
recentlyFencedAndDeletedLedgers.getIfPresent(ledgerId) != null) {
throw
BookieException.create(BookieException.Code.LedgerFencedAndDeletedException);
}
handle = LedgerDescriptor.create(masterKey, ledgerId,
ledgerStorage);
ledgers.putIfAbsent(ledgerId, handle);
}
}
}
```
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java:
##########
@@ -53,16 +53,18 @@ class HandleFactoryImpl implements HandleFactory,
LedgerDeletionListener {
@Override
public LedgerDescriptor getHandle(final long ledgerId, final byte[]
masterKey, boolean journalReplay)
throws IOException, BookieException {
- LedgerDescriptor handle = ledgers.get(ledgerId);
-
- if (handle == null) {
- if (!journalReplay &&
recentlyFencedAndDeletedLedgers.getIfPresent(ledgerId) != null) {
- throw
BookieException.create(BookieException.Code.LedgerFencedAndDeletedException);
+ if (!ledgers.containsKey(ledgerId)) {
Review Comment:
The `containsKey` method in ConcurrentLongHashMap internally calls `get(key)
!= null` (line 221 of ConcurrentLongHashMap.java). This means we're doing two
lookups in the fast path - one for `containsKey` and another for `get` at line
67. Consider using `computeIfAbsent` instead of the double-checked locking
pattern for better performance and cleaner code.
--
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]
