ferdelyi commented on code in PR #8556:
URL: https://github.com/apache/hadoop/pull/8556#discussion_r3644545516
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/logaggregation/filecontroller/ifile/LogAggregationIndexedFileController.java:
##########
@@ -121,22 +121,41 @@ public class LogAggregationIndexedFileController
private int fsNumRetries = 3;
private long fsRetryInterval = 1000L;
private static final int VERSION = 1;
- private IndexedLogsMeta indexedLogsMeta = null;
- private IndexedPerAggregationLogMeta logsMetaInThisCycle;
- private long logAggregationTimeInThisCycle;
- private FSDataOutputStream fsDataOStream;
private Algorithm compressAlgo;
- private CachedIndexedLogsMeta cachedIndexedLogsMeta = null;
- private boolean logAggregationSuccessfullyInThisCyCle = false;
- private long currentOffSet = 0;
- private Path remoteLogCheckSumFile;
- private FileContext fc;
- private UserGroupInformation ugi;
- private byte[] uuid = null;
private final int UUID_LENGTH = 32;
private long logRollOverMaxFileSize;
private Clock sysClock;
+ /**
+ * All mutable state that belongs to a single write session
+ * (one {@link #initializeWriter} / {@link #write} / {@link #postWrite} /
+ * {@link #closeWriter} lifecycle). Bundling it here means the read path
+ * cannot accidentally touch write-path state, regardless of whether the
+ * controller instance is shared across applications.
+ */
+ private static final class WriteSession {
+ /** UUID derived from the application being written. */
+ private final byte[] uuid;
+ /** Accumulated log metadata for the current aggregated file. */
+ private IndexedLogsMeta indexedLogsMeta;
+ /** Log metadata accumulated within this single write cycle. */
+ private IndexedPerAggregationLogMeta logsMetaInThisCycle;
+ private long logAggregationTimeInThisCycle;
+ private boolean logAggregationSuccessfullyInThisCyCle = false;
+ private long currentOffSet = 0;
+ private Path remoteLogCheckSumFile;
+ private FileContext fc;
+ private UserGroupInformation ugi;
+ private FSDataOutputStream fsDataOStream;
+
+ WriteSession(byte[] uuid) {
+ this.uuid = uuid;
+ }
+ }
+
+ /** Non-null while a write session is in progress; null otherwise. */
+ private WriteSession writeSession = null;
Review Comment:
Thank you for your repeated review! It seems to be that the comment was
misleading and it's been updated now. Run a code review specifically for
concurrency hazards and this was the result of the analysis:
Two residual hazards
1. this.writeSession is an unsynchronized mutable field shared between the
write path and closeWriter
closeWriter reads writeSession to close the stream. If — hypothetically — a
second thread called initializeWriter while closeWriter was running
(overwriting writeSession), the first session's stream would not be closed. In
current usage this cannot happen (single-threaded write lifecycle), but the
field is not volatile and has no happens-before guarantee. If the JVM ever
reorders the this.writeSession = session assignment in initializeWriter
relative to the caller seeing it, the caller could use a partially-constructed
session. Again, not possible with the current single-threaded caller, but not
enforced by the type system.
2. this.compressAlgo, this.logRollOverMaxFileSize, this.sysClock,
this.fsNumRetries, this.fsRetryInterval are instance fields set in initInternal
and then read by the write path
These are set once during initialization and only read afterward, so they
are safe in practice. However, they are not final and not volatile, so there is
technically no Java Memory Model guarantee that a thread reading them will see
the initialized values unless there is a happens-before edge between
initInternal and the first use. In practice the framework always calls
initialize() before the controller is handed to any thread, so this is a
theoretical rather than practical issue.
---
Bottom line
The current code has no real concurrency bugs in any deployment scenario
that exists today. The two hazards above are theoretical — both stem from the
fact that the class was never designed for concurrent write access and still
isn't. The comment in initializeWriter now accurately documents this. If the
class were ever used in a context where concurrent writes were possible,
writeSession would need to be volatile (or the field replaced with a
thread-local, or the write methods synchronized), and the initialization fields
would need to be final. For the current use cases, no changes are needed.
--
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]
