mynameborat commented on a change in pull request #1474:
URL: https://github.com/apache/samza/pull/1474#discussion_r592578156
##########
File path:
samza-log4j2/src/main/java/org/apache/samza/logging/log4j2/StreamAppender.java
##########
@@ -75,16 +75,16 @@
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 SOURCE = "log4j-log";
// Hidden config for now. Will move to appropriate Config class when ready
to.
private static final String CREATE_STREAM_ENABLED =
"task.log4j.create.stream.enabled";
private static final long DEFAULT_QUEUE_TIMEOUT_S = 2; // Abitrary choice
private final BlockingQueue<byte[]> logQueue = new
LinkedBlockingQueue<>(DEFAULT_QUEUE_SIZE);
- private SystemStream systemStream = null;
- private SystemProducer systemProducer = null;
+ protected SystemStream systemStream = null;
+ protected SystemProducer systemProducer = null;
Review comment:
Just to reiterate my comment on the other
[PR](https://github.com/apache/samza/pull/1470) for protected changes. I see
multiple attributes being exposed here where in reality you probably only need
few attributes like source, key that needs to be overridden but still keeping
the logic of sending the event to underlying producer same.
The way I see it is not about one liner methods rather conceptual overrides
that extenders need to provide. This way it still avoids code duplication,
unnecessary exposure (against closed principle) and also adds burden to reason
about all the exposed attributes.
e.g. what state should this attributes be before sending event and potential
differences in the implementors perception into those contracts.
TLDR; I feel you should have overrides for what you need as opposed to
exposing all that is required to perform a functionality if the functionality
is common enough across the hierarchy.
----------------------------------------------------------------
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:
[email protected]
