[GitHub] [samza] lakshmi-manasa-g commented on a change in pull request #1403: SAMZA-2569: Add features into StreamAppender

[GitBox <gi...@apache.org>]

lakshmi-manasa-g commented on a change in pull request #1403:
URL: https://github.com/apache/samza/pull/1403#discussion_r461737763



##########
File path: samza-log4j2/src/main/java/org/apache/samza/logging/log4j2/StreamAppender.java
##########
@@ -67,48 +69,52 @@
 import org.apache.samza.system.SystemStream;
 import org.apache.samza.util.ExponentialSleepStrategy;
 import org.apache.samza.util.HttpUtil;
+import org.apache.samza.util.MetricsReporterLoader;
 import org.apache.samza.util.ReflectionUtil;
 
 @Plugin(name = "Stream", category = "Core", elementType = "appender", printObject = true)
 public class StreamAppender extends AbstractAppender {
 
-  private static final String JAVA_OPTS_CONTAINER_NAME = "samza.container.name";
-  private static final String JOB_COORDINATOR_TAG = "samza-job-coordinator";
-  private static final String SOURCE = "log4j-log";
+  protected static final String JAVA_OPTS_CONTAINER_NAME = "samza.container.name";
+  protected static final String JOB_COORDINATOR_TAG = "samza-job-coordinator";
+  protected static final String SOURCE = "log4j-log";

Review comment:
       within Samza i see classes which have only some of their variables exposed as protected and keep the rest private. So i think it is an acceptable pattern to have partial protected. We can expose more as the need arises.
   
   So the current need to extend is coming from having a usecase to support another transformation right .. this PR can "make the parts extendable for different transformation/serde" and that i believe will be acceptable too.

##########
File path: samza-log4j2/src/main/java/org/apache/samza/logging/log4j2/StreamAppender.java
##########
@@ -214,7 +220,7 @@ public void append(LogEvent event) {
           metrics.bufferFillPct.set(Math.round(100f * logQueue.size() / DEFAULT_QUEUE_SIZE));
         }
       } catch (Exception e) {
-        System.err.println("[StreamAppender] Error sending log message:");
+        System.err.println(String.format("[%s] Error sending log message:", appenderName));

Review comment:
       I understand that log4j2.xml should have the Java class name of the appender.
   my question was if we can drop the variable "appenderName" and get it from `getClass().getName` instead




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org