lakshmi-manasa-g commented on a change in pull request #1532:
URL: https://github.com/apache/samza/pull/1532#discussion_r715158025
##########
File path:
samza-log4j2/src/main/java/org/apache/samza/logging/log4j2/StreamAppender.java
##########
@@ -152,18 +144,19 @@ public String getStreamName() {
}
/**
- * Getter for the Config parameter.
+ * This should only be called after verifying that the {@link
LoggingContextHolder} has the config.
Review comment:
there is actually no tangible way to enforce it. Even prior to this PR,
there were only checks to see if the AM's config is available via
"JobModelManager.currentJobModelManager() != null" before using the config or
calling this getConfig() and no real enforcement. But at that time, there was
no explicit comment/doc stating this requirement/caution. now the same thing
happens but via "this.loggingContextHolder.getConfig() != null".
the thing about StreamAppender is that because its a log appender but
depends on non-logging components' flow and availability (for config..) its
flow becomes inter-twined with those components and hence hard to follow. For
example, config get fetched after some component is up and config is asked for
within setupSystem which sits as a no-op until config is avail. in other words,
the internal methods within this class are dependent and called by other
methods BASED on external components flow. This is what lends to this confusing
pattern.
--
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]
