jia-gao commented on code in PR #1652:
URL: https://github.com/apache/samza/pull/1652#discussion_r1092593864
##########
samza-log4j2/src/main/java/org/apache/samza/logging/log4j2/StreamAppender.java:
##########
@@ -186,13 +190,20 @@ public void append(LogEvent event) {
if (!systemInitialized) {
// configs are needed to set up producer system, so check that before
actually initializing
if (this.loggingContextHolder.getConfig() != null) {
- synchronized (this) {
- if (!systemInitialized) {
- setupSystem();
- systemInitialized = true;
+ if (setUpSystemLock.tryLock(SET_UP_SYSTEM_TIMEOUT_MILLI_SECONDS,
TimeUnit.MILLISECONDS)) {
+ try {
+ if (!systemInitialized) {
+ setupSystem();
Review Comment:
Based on the discussion offline:
Yes, `setupSystem` does not provide idempotence nor retry logic to setup
system upon failures.
The current implementation handles both by putting
setupSystem() and systemInitialized = true
into a critical section to ensure that
1. no concurrent calls to setupSystem()
2. If setupSystem() completes, it won't be called again
3. If setupSystem() fails, it will be invoked by another thread (or the same
thread later) that enters the critical section
However, 2 and 3 might not be the ideal way to handle idempotence and
recovery from failures. We should revisit their reliabilities and consider
other solutions
one preferred option is that:
setupSystem() should handle idempotence and recovery from failures by itself
and StreamAppender doesn't need to handle them explicitly in append().
--
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]
