You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:41:47 UTC
[GitHub] [samza] byjiang1996 commented on a change in pull request #1403: Make StreamAppender extend-friendly and add logBytes, logCount metrics, and metrics reporter into StreamAppender
byjiang1996 commented on a change in pull request #1403:
URL: https://github.com/apache/samza/pull/1403#discussion_r461238486
##########
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:
Yes. it will be the same as what we write in log4j2.xml
e.g.
```
<Appender type="Stream" name="<whatismyname>">
<Layout type="PatternLayout" pattern="xxx"/>
</Appender>
```
Then, the appenderName is <whatismyname>. This name should be unique in the log4j2.xml.
##########
File path: samza-log4j2/src/test/java/org/apache/samza/logging/log4j2/TestStreamAppender.java
##########
@@ -68,13 +68,12 @@ public void testDefaultSerde() {
@Test
public void testNonDefaultSerde() {
System.setProperty("samza.container.name", "samza-container-1");
- String streamName = StreamAppender.getStreamName("log4jTest", "1");
Map<String, String> map = new HashMap<String, String>();
map.put("job.name", "log4jTest");
map.put("job.id", "1");
map.put("serializers.registry.log4j-string.class", LoggingEventStringSerdeFactory.class.getCanonicalName());
map.put("systems.mock.samza.factory", MockSystemFactory.class.getCanonicalName());
- map.put("systems.mock.streams." + streamName + ".samza.msg.serde", "log4j-string");
+ map.put("systems.mock.streams.__samza_log4jTest_1_logs.samza.msg.serde", "log4j-string");
Review comment:
Because `getStreamName` function is changed from `protected static ` to `protected` as static method cannot be overridden.
BTW, this static use case only exists here in samza's code. Seems good to remove static keyword.
##########
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:
In my use case (extend existing StreamAppender), I do use small pieces of the changed variables. Just in case in the future, we want to expose more objects or just make extend-friendly feature thoroughly, I prefer to modify all necessary variables (seem useful for child classes) to protected instead of only changing the variables that I intend to use.
##########
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:
Seems appenderName is better to be used here because theoretically it is possible that multiple instances of StreamAppender (or its child appenders) are used at the same time. But appenderName is really a unique name for different appender instances which is set in log4j2.xml
##########
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:
Seems appenderName is better to be used here because theoretically it is possible that multiple instances of StreamAppender (or its child appenders) are used at the same time. But appenderName is really a unique name for different appender instances (it does not need to be the same as "StreamAppender"; it can be any name if users want) which is set in log4j2.xml
##########
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:
@bkonold Do you have ideas about this issue?
----------------------------------------------------------------
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