-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: clarify key or value in (de)serialization processing log messages #6109
Changes from 2 commits
6633890
b63c296
b47e947
9fc4dec
2c828f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,7 @@ | |
import static java.util.Objects.requireNonNull; | ||
|
||
import io.confluent.ksql.logging.processing.ProcessingLogMessageSchema.MessageType; | ||
import io.confluent.ksql.util.ErrorMessageUtil; | ||
import java.util.Base64; | ||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.Optional; | ||
import org.apache.kafka.connect.data.SchemaAndValue; | ||
|
@@ -31,15 +29,18 @@ public class DeserializationError implements ProcessingLogger.ErrorMessage { | |
private final Throwable exception; | ||
private final Optional<byte[]> record; | ||
private final String topic; | ||
private final boolean isKey; | ||
|
||
public DeserializationError( | ||
final Throwable exception, | ||
final Optional<byte[]> record, | ||
final String topic | ||
final String topic, | ||
final boolean isKey | ||
) { | ||
this.exception = requireNonNull(exception, "exception"); | ||
this.record = requireNonNull(record, "record"); | ||
this.topic = requireNonNull(topic, "topic"); | ||
this.isKey = isKey; | ||
} | ||
|
||
@Override | ||
|
@@ -62,27 +63,29 @@ public boolean equals(final Object o) { | |
final DeserializationError that = (DeserializationError) o; | ||
return Objects.equals(exception, that.exception) | ||
&& Objects.equals(record, that.record) | ||
&& Objects.equals(topic, that.topic); | ||
&& Objects.equals(topic, that.topic) | ||
&& isKey == that.isKey; | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(exception, record, topic); | ||
return Objects.hash(exception, record, topic, isKey); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should have a unit test testing this. Can you add one please? |
||
} | ||
|
||
private Struct deserializationError(final ProcessingLogConfig config) { | ||
final Struct deserializationError = new Struct(MessageType.DESERIALIZATION_ERROR.getSchema()) | ||
.put( | ||
ProcessingLogMessageSchema.DESERIALIZATION_ERROR_FIELD_COMPONENT, | ||
LoggingSerdeUtil.getRecordComponent(isKey)) | ||
.put( | ||
ProcessingLogMessageSchema.DESERIALIZATION_ERROR_FIELD_MESSAGE, | ||
exception.getMessage()) | ||
.put( | ||
ProcessingLogMessageSchema.DESERIALIZATION_ERROR_FIELD_CAUSE, | ||
getCause() | ||
) | ||
LoggingSerdeUtil.getCause(exception)) | ||
.put( | ||
ProcessingLogMessageSchema.DESERIALIZATION_ERROR_FIELD_TOPIC, | ||
topic | ||
); | ||
topic); | ||
|
||
if (config.getBoolean(ProcessingLogConfig.INCLUDE_ROWS)) { | ||
deserializationError.put( | ||
|
@@ -93,10 +96,4 @@ private Struct deserializationError(final ProcessingLogConfig config) { | |
|
||
return deserializationError; | ||
} | ||
|
||
private List<String> getCause() { | ||
final List<String> cause = ErrorMessageUtil.getErrorMessages(exception); | ||
cause.remove(0); | ||
return cause; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
/* | ||
* Copyright 2020 Confluent Inc. | ||
* | ||
* Licensed under the Confluent Community License (the "License"); you may not use | ||
* this file except in compliance with the License. You may obtain a copy of the | ||
* License at | ||
* | ||
* http://www.confluent.io/confluent-community-license | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
* WARRANTIES OF ANY KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations under the License. | ||
*/ | ||
|
||
package io.confluent.ksql.logging.processing; | ||
|
||
import io.confluent.ksql.util.ErrorMessageUtil; | ||
import java.util.List; | ||
|
||
final class LoggingSerdeUtil { | ||
|
||
private LoggingSerdeUtil() { | ||
} | ||
|
||
static List<String> getCause(final Throwable exception) { | ||
final List<String> cause = ErrorMessageUtil.getErrorMessages(exception); | ||
cause.remove(0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is a cut & paste, but I've seen this Personally, I'd remove the Which would only leave There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first error message (the one being removed by this line) is already captured in the |
||
return cause; | ||
} | ||
|
||
static String getRecordComponent(final boolean isKey) { | ||
return isKey ? "key" : "value"; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,8 +18,6 @@ | |
import static java.util.Objects.requireNonNull; | ||
|
||
import io.confluent.ksql.logging.processing.ProcessingLogMessageSchema.MessageType; | ||
import io.confluent.ksql.util.ErrorMessageUtil; | ||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.Optional; | ||
import org.apache.kafka.connect.data.SchemaAndValue; | ||
|
@@ -30,15 +28,18 @@ public class SerializationError<T> implements ProcessingLogger.ErrorMessage { | |
private final Throwable exception; | ||
private final Optional<T> record; | ||
private final String topic; | ||
private final boolean isKey; | ||
|
||
public SerializationError( | ||
final Throwable exception, | ||
final Optional<T> record, | ||
final String topic | ||
final String topic, | ||
final boolean isKey | ||
) { | ||
this.exception = requireNonNull(exception, "exception"); | ||
this.record = requireNonNull(record, "record"); | ||
this.topic = requireNonNull(topic, "topic"); | ||
this.isKey = isKey; | ||
} | ||
|
||
@Override | ||
|
@@ -58,30 +59,32 @@ public boolean equals(final Object o) { | |
if (o == null || getClass() != o.getClass()) { | ||
return false; | ||
} | ||
final SerializationError<?> that = (SerializationError) o; | ||
final SerializationError<?> that = (SerializationError<?>) o; | ||
return Objects.equals(exception, that.exception) | ||
&& Objects.equals(record, that.record) | ||
&& Objects.equals(topic, that.topic); | ||
&& Objects.equals(topic, that.topic) | ||
&& isKey == that.isKey; | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(exception, record, topic); | ||
return Objects.hash(exception, record, topic, isKey); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should have a unit test testing this. Can you add one please? |
||
} | ||
|
||
private Struct serializationError(final ProcessingLogConfig config) { | ||
final Struct serializationError = new Struct(MessageType.SERIALIZATION_ERROR.getSchema()) | ||
.put( | ||
ProcessingLogMessageSchema.SERIALIZATION_ERROR_FIELD_COMPONENT, | ||
LoggingSerdeUtil.getRecordComponent(isKey)) | ||
.put( | ||
ProcessingLogMessageSchema.SERIALIZATION_ERROR_FIELD_MESSAGE, | ||
exception.getMessage()) | ||
.put( | ||
ProcessingLogMessageSchema.SERIALIZATION_ERROR_FIELD_CAUSE, | ||
getCause() | ||
) | ||
LoggingSerdeUtil.getCause(exception)) | ||
.put( | ||
ProcessingLogMessageSchema.SERIALIZATION_ERROR_FIELD_TOPIC, | ||
topic | ||
); | ||
topic); | ||
|
||
if (config.getBoolean(ProcessingLogConfig.INCLUDE_ROWS)) { | ||
serializationError.put( | ||
|
@@ -92,10 +95,4 @@ private Struct serializationError(final ProcessingLogConfig config) { | |
|
||
return serializationError; | ||
} | ||
|
||
private List<String> getCause() { | ||
final List<String> cause = ErrorMessageUtil.getErrorMessages(exception); | ||
cause.remove(0); | ||
return cause; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of the name "component" but couldn't think of anything better. Hopefully someone else has a better suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I poked around a bit and didn't see consensus on a good term, but I did see a few sources using the very technical term "part", like "key-part" and "value-part".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't mind component or part, or how about target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, updated to "target".