Skip to content

[fix](editlog) Fix BDBJEJournal.write() burning journal IDs on failure #62063

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.

Sign up for GitHub

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

Closed
dataroaring wants to merge 1 commit into from
Closed

[fix](editlog) Fix BDBJEJournal.write() burning journal IDs on failure #62063

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions fe/fe-core/src/main/java/org/apache/doris/journal/bdbje/BDBJEJournal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// Write the journals to bdb.
for (int i = 0; i < RETRY_TIME; i++) {
Expand All @@ -155,6 +156,7 @@ public synchronized long write(JournalBatch batch) throws IOException {

txn.commit();
txn = null;
nextJournalId.addAndGet(entitySize);
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

if (MetricRepo.isInit) {
MetricRepo.COUNTER_EDIT_LOG_SIZE_BYTES.increase(dataSize);
Expand Down Expand Up @@ -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.
Comment on lines +242 to +243
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// id is the key. Reserve ID only after successful write to avoid burning IDs on failure.
// This is safe because the method is synchronized.
// 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.

Copilot uses AI. Check for mistakes.
long id = nextJournalId.get();
DatabaseEntry theKey = idToKey(id);

// entity is the value
Expand Down Expand Up @@ -273,6 +276,7 @@ public synchronized long write(short op, Writable writable) throws IOException {
// Parameter null means auto commit
if (currentJournalDB.put(null, theKey, theData) == OperationStatus.SUCCESS) {
writeSucceed = true;
nextJournalId.incrementAndGet();
if (LOG.isDebugEnabled()) {
LOG.debug("master write journal {} finished. db name {}, current time {}",
id, currentJournalDB.getDatabaseName(), System.currentTimeMillis());
Expand Down
Loading