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 2022/11/08 17:23:03 UTC
[GitHub] [samza] jia-gao opened a new pull request, #1641: LISAMZA-27395 removing the current recursive call prevention logic
jia-gao opened a new pull request, #1641:
URL: https://github.com/apache/samza/pull/1641
Issue:
We have observed logging data loss happen in StreamAppender
The RC is that current implementation of the “[append()](https://github.com/apache/samza/blob/master/samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java#L146)” method uses one AtomicBoolean variable to detect if the thread is called recursively.
However, it won’t work as expected since Log4j2 framework already applies recursive call prevention and it has the side effect that data loss might occur in the parallel logging scenario
Change:
Remove current recursive call prevention logic, Leverage [log4j2 framework](https://github.com/apache/logging-log4j2/blob/release-2.x/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AppenderControl.java#L122) to prevent recursive calls
API Changes:
Remove a metric recursivecall from StreamAppenderMetrics since it is no longer used
Test Done:
./gradlew build
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] shanthoosh commented on pull request #1641: LISAMZA-27395 removing the current recursive call prevention logic
Posted by GitBox <gi...@apache.org>.
shanthoosh commented on PR #1641:
URL: https://github.com/apache/samza/pull/1641#issuecomment-1317938599
Thanks for adding the tests @jia-gao . After going through the linkedin documents and JIRA tickets, it seems like we are just proceeding with the approach finalized as a part of the investigation.
We had just removed the recursive call check from the log4j2-appender implementation and we're relying upon native-recursive check from log4j2. Mostly the changes look good to me, let us wait for the build to pass and we can merge this in!
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on pull request #1641: LISAMZA-27395 removing the current recursive call prevention logic
Posted by GitBox <gi...@apache.org>.
mynameborat commented on PR #1641:
URL: https://github.com/apache/samza/pull/1641#issuecomment-1307762089
Is the default support to prevent recursion not available when this change was introduced or did we upgrade to newer version of log4j that adds this prevention at the framework layer?
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] jia-gao commented on pull request #1641: LISAMZA-27395 removing the current recursive call prevention logic
Posted by GitBox <gi...@apache.org>.
jia-gao commented on PR #1641:
URL: https://github.com/apache/samza/pull/1641#issuecomment-1314166098
> Is the default support to prevent recursion not available when this change was introduced or did we upgrade to newer version of log4j that adds this prevention at the framework layer?
log4j1 doesn't have this prevention that's when we added the change in samza. Later we upgraded to log4j2 which has he prevention at the framework layer.
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] shanthoosh commented on pull request #1641: LISAMZA-27395 removing the current recursive call prevention logic
Posted by GitBox <gi...@apache.org>.
shanthoosh commented on PR #1641:
URL: https://github.com/apache/samza/pull/1641#issuecomment-1317817165
@jia-gao As discussed offline, it would be great if we can add a unit-test for validating these changes in this patch.
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] jia-gao commented on a diff in pull request #1641: LISAMZA-27395 removing the current recursive call prevention logic
Posted by GitBox <gi...@apache.org>.
jia-gao commented on code in PR #1641:
URL: https://github.com/apache/samza/pull/1641#discussion_r1024297945
##########
samza-log4j2/src/main/java/org/apache/samza/logging/log4j2/StreamAppenderMetrics.java:
##########
@@ -46,7 +43,6 @@ public class StreamAppenderMetrics extends MetricsBase {
public StreamAppenderMetrics(String prefix, MetricsRegistry registry) {
super(prefix + "-", registry);
bufferFillPct = newGauge("buffer-fill-percent", 0);
- recursiveCalls = newCounter("recursive-calls");
Review Comment:
Didn't find StreamAppenderMetrics published in any oss samza doc
https://samza.apache.org/learn/documentation/latest/container/metrics-table.html
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] jia-gao commented on pull request #1641: LISAMZA-27395 removing the current recursive call prevention logic
Posted by GitBox <gi...@apache.org>.
jia-gao commented on PR #1641:
URL: https://github.com/apache/samza/pull/1641#issuecomment-1317936984
> validating
> @jia-gao As discussed offline, it would be great if we can add a unit-test for validating these changes in this patch.
Added unit tests
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] alnzng commented on a diff in pull request #1641: LISAMZA-27395 removing the current recursive call prevention logic
Posted by GitBox <gi...@apache.org>.
alnzng commented on code in PR #1641:
URL: https://github.com/apache/samza/pull/1641#discussion_r1021917783
##########
samza-log4j2/src/main/java/org/apache/samza/logging/log4j2/StreamAppenderMetrics.java:
##########
@@ -46,7 +43,6 @@ public class StreamAppenderMetrics extends MetricsBase {
public StreamAppenderMetrics(String prefix, MetricsRegistry registry) {
super(prefix + "-", registry);
bufferFillPct = newGauge("buffer-fill-percent", 0);
- recursiveCalls = newCounter("recursive-calls");
Review Comment:
is this metric published in some Samza doc? If yes, let's remove it from there as well.
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] shanthoosh merged pull request #1641: LISAMZA-27395 removing the current recursive call prevention logic
Posted by GitBox <gi...@apache.org>.
shanthoosh merged PR #1641:
URL: https://github.com/apache/samza/pull/1641
--
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: commits-unsubscribe@samza.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org