[
https://issues.apache.org/jira/browse/BEAM-6013?focusedWorklogId=164073&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-164073
]
ASF GitHub Bot logged work on BEAM-6013:
----------------------------------------
Author: ASF GitHub Bot
Created on: 08/Nov/18 20:28
Start Date: 08/Nov/18 20:28
Worklog Time Spent: 10m
Work Description: lukecwik closed pull request #6982: [BEAM-6013] Reduce
logging within SerializableCoder.
URL: https://github.com/apache/beam/pull/6982
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/SerializableCoder.java
b/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/SerializableCoder.java
index 87a75eefcab..d40cd381dbc 100644
---
a/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/SerializableCoder.java
+++
b/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/SerializableCoder.java
@@ -25,7 +25,10 @@
import java.io.OutputStream;
import java.io.Serializable;
import java.lang.reflect.Method;
+import java.util.Collections;
import java.util.List;
+import java.util.Set;
+import java.util.WeakHashMap;
import javax.annotation.Nullable;
import org.apache.beam.sdk.PipelineRunner;
import org.apache.beam.sdk.values.TypeDescriptor;
@@ -49,6 +52,13 @@
*/
public class SerializableCoder<T extends Serializable> extends CustomCoder<T> {
+ /*
+ * A thread safe set containing classes which we have warned about.
+ * Note that we specifically use a weak hash map to allow for classes to be
unloaded.
+ */
+ private static final Set<Class<?>> MISSING_EQUALS_METHOD =
+ Collections.synchronizedSet(Collections.newSetFromMap(new
WeakHashMap<>()));
+
private static final Logger LOG =
LoggerFactory.getLogger(SerializableCoder.class);
/**
@@ -87,8 +97,8 @@ public Object structuralValue(T value) {
}
private static <T extends Serializable> void
checkEqualsMethodDefined(Class<T> clazz) {
- boolean warn = clazz.isInterface();
- if (!warn) {
+ boolean warn = true;
+ if (!clazz.isInterface()) {
Method method;
try {
method = clazz.getMethod("equals", Object.class);
@@ -99,7 +109,10 @@ public Object structuralValue(T value) {
// Check if not default Object#equals implementation.
warn = Object.class.equals(method.getDeclaringClass());
}
- if (warn) {
+
+ // Note that the order of these checks is important since we want the
+ // "did we add the class to the set" check to happen last.
+ if (warn && MISSING_EQUALS_METHOD.add(clazz)) {
LOG.warn(
"Can't verify serialized elements of type {} have well defined
equals method. "
+ "This may produce incorrect results on some {}",
diff --git
a/sdks/java/core/src/test/java/org/apache/beam/sdk/coders/SerializableCoderTest.java
b/sdks/java/core/src/test/java/org/apache/beam/sdk/coders/SerializableCoderTest.java
index 66019f3f580..70817c0a13f 100644
---
a/sdks/java/core/src/test/java/org/apache/beam/sdk/coders/SerializableCoderTest.java
+++
b/sdks/java/core/src/test/java/org/apache/beam/sdk/coders/SerializableCoderTest.java
@@ -31,6 +31,8 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import java.util.logging.Level;
+import java.util.logging.LogRecord;
import org.apache.beam.sdk.testing.CoderProperties;
import org.apache.beam.sdk.testing.ExpectedLogs;
import org.apache.beam.sdk.testing.NeedsRunner;
@@ -43,7 +45,9 @@
import org.apache.beam.sdk.util.SerializableUtils;
import org.apache.beam.sdk.values.PCollection;
import org.apache.beam.sdk.values.TypeDescriptor;
+import org.hamcrest.Description;
import org.hamcrest.Matchers;
+import org.hamcrest.TypeSafeMatcher;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
@@ -252,19 +256,65 @@ public void testSerializableCoderProviderIsRegistered()
throws Exception {
@Test
public void coderWarnsForInterface() throws Exception {
- SerializableCoder.of(TestInterface.class);
- expectedLogs.verifyWarn(
+ String expectedLogMessage =
"Can't verify serialized elements of type TestInterface "
- + "have well defined equals method.");
+ + "have well defined equals method.";
+ // Create the coder multiple times ensuring that we only log once.
+ SerializableCoder.of(TestInterface.class);
+ SerializableCoder.of(TestInterface.class);
+ SerializableCoder.of(TestInterface.class);
+ expectedLogs.verifyLogRecords(
+ new TypeSafeMatcher<Iterable<LogRecord>>() {
+ @Override
+ public void describeTo(Description description) {
+ description.appendText(
+ String.format("single warn log message containing [%s]",
expectedLogMessage));
+ }
+
+ @Override
+ protected boolean matchesSafely(Iterable<LogRecord> item) {
+ int count = 0;
+ for (LogRecord logRecord : item) {
+ if (logRecord.getLevel().equals(Level.WARNING)
+ && logRecord.getMessage().contains(expectedLogMessage)) {
+ count += 1;
+ }
+ }
+ return count == 1;
+ }
+ });
}
private static class NoEquals implements Serializable {}
@Test
public void coderWarnsForNoEquals() throws Exception {
+ String expectedLogMessage =
+ "Can't verify serialized elements of type NoEquals " + "have well
defined equals method.";
+ // Create the coder multiple times ensuring that we only log once.
+ SerializableCoder.of(NoEquals.class);
+ SerializableCoder.of(NoEquals.class);
SerializableCoder.of(NoEquals.class);
- expectedLogs.verifyWarn(
- "Can't verify serialized elements of type NoEquals " + "have well
defined equals method.");
+ expectedLogs.verifyLogRecords(
+ new TypeSafeMatcher<Iterable<LogRecord>>() {
+ @Override
+ public void describeTo(Description description) {
+ description.appendText(
+ String.format("single warn log message containing [%s]",
expectedLogMessage));
+ }
+
+ @Override
+ protected boolean matchesSafely(Iterable<LogRecord> item) {
+ int count = 0;
+ for (LogRecord logRecord : item) {
+ if (logRecord.getLevel().equals(Level.WARNING)
+ && logRecord.getMessage().contains(expectedLogMessage)) {
+ count += 1;
+ }
+ }
+ return count == 1;
+ }
+ });
}
private static class ProperEquals implements Serializable {
diff --git
a/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/ExpectedLogs.java
b/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/ExpectedLogs.java
index a79c5e635e8..ad976531620 100644
--- a/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/ExpectedLogs.java
+++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/ExpectedLogs.java
@@ -175,6 +175,17 @@ public void verifyNoError(String substring, Throwable t) {
verifyNo(Level.SEVERE, substring, t);
}
+ /**
+ * Verify that the list of log records matches the provided {@code matcher}.
+ *
+ * @param matcher The matcher to use.
+ */
+ public void verifyLogRecords(Matcher<Iterable<LogRecord>> matcher) {
+ if (!matcher.matches(logSaver.getLogs())) {
+ fail(String.format("Missing match for [%s]", matcher));
+ }
+ }
+
private void verify(final Level level, final String substring) {
verifyLogged(matcher(level, substring));
}
diff --git
a/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/ExpectedLogsTest.java
b/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/ExpectedLogsTest.java
index 1d0303fc3bc..896ca6958ec 100644
---
a/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/ExpectedLogsTest.java
+++
b/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/ExpectedLogsTest.java
@@ -28,6 +28,8 @@
import java.util.concurrent.ExecutorCompletionService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
+import java.util.logging.LogRecord;
+import org.hamcrest.TypeSafeMatcher;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.Description;
@@ -72,6 +74,29 @@ public void testVerifyWhenMatchedWithExceptionBeingLogged()
throws Throwable {
expectedLogs.verifyError(expected);
}
+ @Test
+ public void testVerifyLogRecords() throws Throwable {
+ String expected = generateRandomString();
+ LOG.error(expected);
+ LOG.error(expected);
+ expectedLogs.verifyLogRecords(
+ new TypeSafeMatcher<Iterable<LogRecord>>() {
+ @Override
+ protected boolean matchesSafely(Iterable<LogRecord> item) {
+ int count = 0;
+ for (LogRecord record : item) {
+ if (record.getMessage().contains(expected)) {
+ count += 1;
+ }
+ }
+ return count == 2;
+ }
+
+ @Override
+ public void describeTo(org.hamcrest.Description description) {}
+ });
+ }
+
@Test(expected = AssertionError.class)
public void testVerifyWhenNotMatched() throws Throwable {
String expected = generateRandomString();
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 164073)
Time Spent: 40m (was: 0.5h)
> Reduce verbose logging within SerializableCoder
> -----------------------------------------------
>
> Key: BEAM-6013
> URL: https://issues.apache.org/jira/browse/BEAM-6013
> Project: Beam
> Issue Type: Improvement
> Components: sdk-java-core
> Reporter: Luke Cwik
> Assignee: Luke Cwik
> Priority: Minor
> Fix For: 2.9.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> The following message is spamming logs constantly:
> "Can't verify serialized elements of type TypeX have well defined equals
> method. This may produce incorrect results on some PipelineRunner"
> Code location:
> https://github.com/apache/beam/blob/429547981b4534a29a0654e3b86f1a718793d816/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/SerializableCoder.java#L104
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
