You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/02/14 20:46:12 UTC
[GitHub] [kafka] C0urante opened a new pull request #11758: MINOR: Clarify logging behavior with errors.log.include.messages property
C0urante opened a new pull request #11758:
URL: https://github.com/apache/kafka/pull/11758
The docs are a little misleading and some users can be confused about the exact behavior of this property.
References:
- [LogReporter::report and LogReporter::message](https://github.com/apache/kafka/blob/b5b590cb6711617cbb3c978de9f8bb0903291efa/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/errors/LogReporter.java#L51-L75)
- [ConnectorConfig::includeRecordDetailsInErrorLog](https://github.com/apache/kafka/blob/b5b590cb6711617cbb3c978de9f8bb0903291efa/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java#L264-L266)
- [ProcessingContext::toString](https://github.com/apache/kafka/blob/b5b590cb6711617cbb3c978de9f8bb0903291efa/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/errors/ProcessingContext.java#L164-L188)
- [SourceRecord::toString](https://github.com/apache/kafka/blob/b5b590cb6711617cbb3c978de9f8bb0903291efa/connect/api/src/main/java/org/apache/kafka/connect/source/SourceRecord.java#L129-L135)
- [ConnectRecord::toString](https://github.com/apache/kafka/blob/b5b590cb6711617cbb3c978de9f8bb0903291efa/connect/api/src/main/java/org/apache/kafka/connect/connector/ConnectRecord.java#L137-L149)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jwiederholdconfluent commented on a change in pull request #11758: MINOR: Clarify logging behavior with errors.log.include.messages property
Posted by GitBox <gi...@apache.org>.
jwiederholdconfluent commented on a change in pull request #11758:
URL: https://github.com/apache/kafka/pull/11758#discussion_r808247925
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java
##########
@@ -150,9 +150,11 @@
public static final String ERRORS_LOG_INCLUDE_MESSAGES_CONFIG = "errors.log.include.messages";
public static final String ERRORS_LOG_INCLUDE_MESSAGES_DISPLAY = "Log Error Details";
public static final boolean ERRORS_LOG_INCLUDE_MESSAGES_DEFAULT = false;
- public static final String ERRORS_LOG_INCLUDE_MESSAGES_DOC = "Whether to the include in the log the Connect record that resulted in " +
- "a failure. This is 'false' by default, which will prevent record keys, values, and headers from being written to log files, " +
- "although some information such as topic and partition number will still be logged.";
+ public static final String ERRORS_LOG_INCLUDE_MESSAGES_DOC = "Whether to the include in the log the Connect record that resulted in a failure." +
+ "For sink records, the topic, partition, offset, and timestamp will be logged. " +
+ "For source records, the key and value (and their schemas), all headers, and the timestamp, Kafka topic, Kafka partition, source partition, " +
Review comment:
The change looks good! Much more clear.
One small thing... should "Whether to the include" be "Whether to include"?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on pull request #11758: MINOR: Clarify logging behavior with errors.log.include.messages property
Posted by GitBox <gi...@apache.org>.
rhauch commented on pull request #11758:
URL: https://github.com/apache/kafka/pull/11758#issuecomment-1048280984
Merged to `trunk` and backported to the `3.1` and `3.0` branches.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #11758: MINOR: Clarify logging behavior with errors.log.include.messages property
Posted by GitBox <gi...@apache.org>.
C0urante commented on pull request #11758:
URL: https://github.com/apache/kafka/pull/11758#issuecomment-1048298478
Thanks Randall, appreciate it.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a change in pull request #11758: MINOR: Clarify logging behavior with errors.log.include.messages property
Posted by GitBox <gi...@apache.org>.
C0urante commented on a change in pull request #11758:
URL: https://github.com/apache/kafka/pull/11758#discussion_r807517678
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java
##########
@@ -150,9 +150,11 @@
public static final String ERRORS_LOG_INCLUDE_MESSAGES_CONFIG = "errors.log.include.messages";
public static final String ERRORS_LOG_INCLUDE_MESSAGES_DISPLAY = "Log Error Details";
public static final boolean ERRORS_LOG_INCLUDE_MESSAGES_DEFAULT = false;
- public static final String ERRORS_LOG_INCLUDE_MESSAGES_DOC = "Whether to the include in the log the Connect record that resulted in " +
- "a failure. This is 'false' by default, which will prevent record keys, values, and headers from being written to log files, " +
- "although some information such as topic and partition number will still be logged.";
+ public static final String ERRORS_LOG_INCLUDE_MESSAGES_DOC = "Whether to the include in the log the Connect record that resulted in a failure." +
+ "For sink records, the topic, partition, offset, and timestamp will be logged. " +
+ "For source records, the key and value (and their schemas), all headers, and the timestamp, Kafka topic, Kafka partition, source partition, " +
Review comment:
High-level, I think this description is already verbose enough and if anything, it should be shortened.
RE "topic name", I think that restricting the definition to just topic names and not identifiers is unnecessary and puts us in an awkward decision if it's ever desirable to include that information in the logged error messages since it's not obvious in that part of the code base that this docstring would need to be updated.
RE "partition number", I think that "partition" is all but completely unambiguous and that suffixing it with "number" is redundant.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch merged pull request #11758: MINOR: Clarify logging behavior with errors.log.include.messages property
Posted by GitBox <gi...@apache.org>.
rhauch merged pull request #11758:
URL: https://github.com/apache/kafka/pull/11758
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a change in pull request #11758: MINOR: Clarify logging behavior with errors.log.include.messages property
Posted by GitBox <gi...@apache.org>.
C0urante commented on a change in pull request #11758:
URL: https://github.com/apache/kafka/pull/11758#discussion_r807517678
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java
##########
@@ -150,9 +150,11 @@
public static final String ERRORS_LOG_INCLUDE_MESSAGES_CONFIG = "errors.log.include.messages";
public static final String ERRORS_LOG_INCLUDE_MESSAGES_DISPLAY = "Log Error Details";
public static final boolean ERRORS_LOG_INCLUDE_MESSAGES_DEFAULT = false;
- public static final String ERRORS_LOG_INCLUDE_MESSAGES_DOC = "Whether to the include in the log the Connect record that resulted in " +
- "a failure. This is 'false' by default, which will prevent record keys, values, and headers from being written to log files, " +
- "although some information such as topic and partition number will still be logged.";
+ public static final String ERRORS_LOG_INCLUDE_MESSAGES_DOC = "Whether to the include in the log the Connect record that resulted in a failure." +
+ "For sink records, the topic, partition, offset, and timestamp will be logged. " +
+ "For source records, the key and value (and their schemas), all headers, and the timestamp, Kafka topic, Kafka partition, source partition, " +
Review comment:
High-level, I think this description is already verbose enough and if anything, it should be shortened.
RE "topic name", I think that restricting the definition to just topic names and not identifiers is unnecessary and puts us in an awkward place if it's ever desirable to include that information in the logged error messages since it's not obvious in that part of the code base that this docstring would need to be updated.
RE "partition number", I think that "partition" is all but completely unambiguous and that suffixing it with "number" is redundant.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on a change in pull request #11758: MINOR: Clarify logging behavior with errors.log.include.messages property
Posted by GitBox <gi...@apache.org>.
rhauch commented on a change in pull request #11758:
URL: https://github.com/apache/kafka/pull/11758#discussion_r807394363
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java
##########
@@ -150,9 +150,11 @@
public static final String ERRORS_LOG_INCLUDE_MESSAGES_CONFIG = "errors.log.include.messages";
public static final String ERRORS_LOG_INCLUDE_MESSAGES_DISPLAY = "Log Error Details";
public static final boolean ERRORS_LOG_INCLUDE_MESSAGES_DEFAULT = false;
- public static final String ERRORS_LOG_INCLUDE_MESSAGES_DOC = "Whether to the include in the log the Connect record that resulted in " +
- "a failure. This is 'false' by default, which will prevent record keys, values, and headers from being written to log files, " +
- "although some information such as topic and partition number will still be logged.";
+ public static final String ERRORS_LOG_INCLUDE_MESSAGES_DOC = "Whether to the include in the log the Connect record that resulted in a failure." +
+ "For sink records, the topic, partition, offset, and timestamp will be logged. " +
+ "For source records, the key and value (and their schemas), all headers, and the timestamp, Kafka topic, Kafka partition, source partition, " +
Review comment:
What do you think about clarifying even more precisely that it's the Kafka topic _name_ and Kafka partition _number_? I know, it's nonsensical to log the topic or partition _contents_, but someone that is still learning how Connect works might have fewer questions. Plus, there is other topic metadata (like [topic IDs](https://cwiki.apache.org/confluence/display/KAFKA/KIP-516%3A+Topic+Identifiers)) that we're not logging.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org