[fix](editlog) Fix EditLog.txId data race by converting to AtomicLong#62059
[fix](editlog) Fix EditLog.txId data race by converting to AtomicLong#62059dataroaring wants to merge 1 commit intoapache:masterfrom
Conversation
The txId field is a plain long that is read and written by multiple threads without synchronization: - The flusher thread increments txId in flushEditLog() (txId += batch.size()) - DDL caller threads increment txId in logEditDirectly() (txId++) - logEdit(op, List) also increments txId (txId += entries.size()) Since long writes are not atomic on all JVMs (JLS §17.7 permits non-atomic 64-bit writes), concurrent read/write can produce torn values. Even on platforms with atomic 64-bit writes, the lack of happens-before ordering means one thread's write may not be visible to another. Fix: convert txId from plain long to AtomicLong, updating all read/write sites to use the appropriate atomic operations.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a data race on EditLog.txId by replacing a plain long with an AtomicLong, ensuring atomic reads/writes across multiple threads interacting with edit log flushing and direct logging.
Changes:
- Convert
txIdfromlongtoAtomicLong. - Update all
txIdincrement/read/reset sites to use atomic operations. - Adjust logging to print the atomic value via
get().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| txId.addAndGet(batch.size()); | ||
| // update statistics, etc. (optional, can be added as needed) | ||
| if (txId >= Config.edit_log_roll_num) { | ||
| LOG.info("txId {} is equal to or larger than edit_log_roll_num {}, will roll edit.", txId, | ||
| if (txId.get() >= Config.edit_log_roll_num) { | ||
| LOG.info("txId {} is equal to or larger than edit_log_roll_num {}, will roll edit.", txId.get(), | ||
| Config.edit_log_roll_num); | ||
| rollEditLog(); | ||
| txId = 0; | ||
| txId.set(0); | ||
| } |
There was a problem hiding this comment.
The get() / rollEditLog() / set(0) sequence is not atomic with respect to concurrent increments. Another thread can increment txId after the threshold check (or during rollEditLog()), and set(0) can then wipe out those increments, causing missed roll accounting and potentially delaying/accelerating future rolls. Consider guarding the roll decision/reset with a CAS pattern (e.g., capture the post-increment value and compareAndSet it back to 0 only if unchanged) or synchronizing the roll/reset with the same lock used for rolling so increments can’t be lost.
| long newTxId = txId.incrementAndGet(); | ||
|
|
||
| if (txId >= Config.edit_log_roll_num) { | ||
| LOG.info("txId {} is equal to or larger than edit_log_roll_num {}, will roll edit.", txId, | ||
| if (newTxId >= Config.edit_log_roll_num) { | ||
| LOG.info("txId {} is equal to or larger than edit_log_roll_num {}, will roll edit.", newTxId, | ||
| Config.edit_log_roll_num); | ||
| rollEditLog(); | ||
| txId = 0; | ||
| txId.set(0); | ||
| } |
There was a problem hiding this comment.
Similar to flushEditLog(), incrementAndGet() followed by rollEditLog() and set(0) can lose increments performed by other threads while the roll is in progress, because the reset is unconditional. If logEditDirectly() can run concurrently with other increment sites, prefer making the reset conditional (CAS) or performing the roll+reset under a shared lock that also covers all txId increments related to rolling.
| txId.addAndGet(batch.size()); | ||
| // update statistics, etc. (optional, can be added as needed) | ||
| if (txId >= Config.edit_log_roll_num) { | ||
| LOG.info("txId {} is equal to or larger than edit_log_roll_num {}, will roll edit.", txId, | ||
| if (txId.get() >= Config.edit_log_roll_num) { | ||
| LOG.info("txId {} is equal to or larger than edit_log_roll_num {}, will roll edit.", txId.get(), |
There was a problem hiding this comment.
txId.get() is called multiple times after an increment, which can make logs harder to interpret under concurrency (the logged value may not match the value used for the decision). Prefer capturing the post-increment value in a local long (e.g., from addAndGet(...)) and using that for both the threshold check and the log message.
Summary
txIdfield is a plainlongread/written by multiple threads (flusher, DDL callers, bulk writers) without synchronizationtxIdtoAtomicLongand update all 6 read/write sitesTest plan
edit_log_roll_numtransactions🤖 Generated with Claude Code