Copilot commented on code in PR #62063:
URL: https://github.com/apache/doris/pull/62063#discussion_r3028464312
##########
fe/fe-core/src/main/java/org/apache/doris/journal/bdbje/BDBJEJournal.java:
##########
@@ -131,7 +131,8 @@ public synchronized long write(JournalBatch batch) throws
IOException {
List<JournalBatch.Entity> entities = batch.getJournalEntities();
int entitySize = entities.size();
long dataSize = 0;
- long firstId = nextJournalId.getAndAdd(entitySize);
+ // Reserve IDs only after successful commit to avoid burning IDs on
write failure.
+ long firstId = nextJournalId.get();
Review Comment:
`nextJournalId.addAndGet(entitySize)` is performed inside the retry loop.
This is safe only if the method exits/breaks immediately after a successful
commit; otherwise a future refactor (or a subtle control-flow change) could
advance the counter multiple times for the same batch. To make this robust,
consider structuring the success path to return/break immediately after
`txn.commit()` (and then advance once), or perform the `addAndGet` in a single
place that is executed exactly once when `writeSucceed` is finalized.
##########
fe/fe-core/src/main/java/org/apache/doris/journal/bdbje/BDBJEJournal.java:
##########
@@ -155,6 +156,7 @@ public synchronized long write(JournalBatch batch) throws
IOException {
txn.commit();
txn = null;
+ nextJournalId.addAndGet(entitySize);
Review Comment:
`nextJournalId.addAndGet(entitySize)` is performed inside the retry loop.
This is safe only if the method exits/breaks immediately after a successful
commit; otherwise a future refactor (or a subtle control-flow change) could
advance the counter multiple times for the same batch. To make this robust,
consider structuring the success path to return/break immediately after
`txn.commit()` (and then advance once), or perform the `addAndGet` in a single
place that is executed exactly once when `writeSucceed` is finalized.
##########
fe/fe-core/src/main/java/org/apache/doris/journal/bdbje/BDBJEJournal.java:
##########
@@ -237,8 +239,9 @@ public synchronized long write(short op, Writable writable)
throws IOException {
entity.setOpCode(op);
entity.setData(writable);
- // id is the key
- long id = nextJournalId.getAndIncrement();
+ // id is the key. Reserve ID only after successful write to avoid
burning IDs on failure.
+ // This is safe because the method is synchronized.
Review Comment:
The comment says “Reserve ID only after successful write”, but the code
still selects the ID before the write; what changes is when `nextJournalId` is
advanced. Consider rewording to something like “Advance `nextJournalId` only
after successful write/commit” to match the actual behavior and avoid confusion
for future maintainers.
```suggestion
// id is the key. We advance nextJournalId only after successful
write/commit
// to avoid burning IDs on failure. This is safe because the method
is synchronized.
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
