[
https://issues.apache.org/jira/browse/MAPREDUCE-7539?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18098836#comment-18098836
]
ASF GitHub Bot commented on MAPREDUCE-7539:
-------------------------------------------
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.
> JobHistoryServer API fails to serve aggregated logs due to uuid mismatch
> ------------------------------------------------------------------------
>
> Key: MAPREDUCE-7539
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-7539
> Project: Hadoop Map/Reduce
> Issue Type: Bug
> Components: jobhistoryserver
> Affects Versions: 3.5.0
> Reporter: Brian Goerlitz
> Assignee: Ferenc Erdelyi
> Priority: Major
> Labels: pull-request-available
>
> Due to Singleton annotations added in HADOOP-15984 for HSWebServices, the
> first time an ifile log is read via the {{/ws/v1/history/aggregatedlogs}}
> API, the UUID of the log is stored in the HSWebServices instance of the
> {{LogAggregationIndexedFileController}} and used for verification of all
> future log files. This results in failure to read any aggregated log files
> belonging to an app that is not the first one accessed after JHS restart.
> {noformat}
> 2026-06-08 20:16:40,368 WARN
> org.apache.hadoop.yarn.logaggregation.filecontroller.ifile.LogAggregationIndexedFileController:
> Can not get log meta from the log
> file:hdfs://nn:8020/tmp/logs/systest/bucket-logs-ifile/0002/application_1780935195539_0002/nm_8041
> The UUID from
> hdfs://nn:8020/tmp/logs/systest/bucket-logs-ifile/0002/application_1780935195539_0002/nm_8041
> is not correct. The offset of loaded UUID is 296605
> 2026-06-08 20:16:40,368 WARN
> org.apache.hadoop.yarn.webapp.GenericExceptionHandler: SERVICE_UNAVAILABLE
> javax.ws.rs.WebApplicationException: HTTP 500 Internal Server Error
> at
> org.apache.hadoop.yarn.server.webapp.LogServlet.getContainerLogMeta(LogServlet.java:134)
> at
> org.apache.hadoop.yarn.server.webapp.LogServlet.getContainerLogsInfo(LogServlet.java:325)
> at
> org.apache.hadoop.yarn.server.webapp.LogServlet.getLogsInfo(LogServlet.java:263)
> at
> org.apache.hadoop.mapreduce.v2.hs.webapp.HsWebServices.getAggregatedLogsMeta(HsWebServices.java:521)
> ...
> Caused by: org.apache.hadoop.yarn.webapp.NotFoundException: HTTP 404 Not Found
> at
> org.apache.hadoop.yarn.server.webapp.LogServlet.getContainerLogMeta(LogServlet.java:122)
> ... 86 more
> Caused by: java.lang.Exception: Can not get log meta for request.
> at
> org.apache.hadoop.yarn.webapp.NotFoundException.<init>(NotFoundException.java:45)
> ... 87 more
> {noformat}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
