ppkarwasz commented on code in PR #836:
URL: https://github.com/apache/logging-log4j2/pull/836#discussion_r867885092
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/StructuredDataId.java:
##########
@@ -127,7 +127,7 @@ public StructuredDataId(final String name, final String[]
required, final String
* @param required The list of keys that are required for this id.
* @param optional The list of keys that are optional for this id.
*/
- public StructuredDataId(final String name, final int enterpriseNumber,
final String[] required,
+ public StructuredDataId(final String name, final String enterpriseNumber,
final String[] required,
Review Comment:
For backward compatibility, the signature of public methods can not be
changed. Just deprecate the old constructor and create a new one.
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/StructuredDataId.java:
##########
@@ -142,15 +142,15 @@ public StructuredDataId(final String name, final int
enterpriseNumber, final Str
* @param maxLength The maximum length of the StructuredData Id key.
* @since 2.9
*/
- public StructuredDataId(final String name, final int enterpriseNumber,
final String[] required,
+ public StructuredDataId(final String name, final String enterpriseNumber,
final String[] required,
Review Comment:
As above, the change must be backward compatible. This comment applies to
all other signature changes of public methods.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -71,9 +71,9 @@
public final class Rfc5424Layout extends AbstractStringLayout {
/**
- * Not a very good default - it is the Apache Software Foundation's
enterprise number.
+ * The default example enterprise number from RFC5424.
*/
- public static final int DEFAULT_ENTERPRISE_NUMBER = 18060;
+ public static final String DEFAULT_ENTERPRISE_NUMBER = "32473";
Review Comment:
I totally agree, `32473` should be the default.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/SyslogAppender.java:
##########
@@ -375,7 +375,7 @@ public static <B extends Builder<B>> SyslogAppender
createAppender(
final boolean ignoreExceptions,
final Facility facility,
final String id,
- final int enterpriseNumber,
+ final String enterpriseNumber,
Review Comment:
This factory method is deprecated. You can leave the old signature.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/layout/Rfc5424Layout.java:
##########
@@ -610,8 +610,8 @@ public static Rfc5424Layout createLayout(
// @formatter:off
@PluginAttribute(value = "facility", defaultString = "LOCAL0")
final Facility facility,
@PluginAttribute("id") final String id,
- @PluginAttribute(value = "enterpriseNumber", defaultInt =
DEFAULT_ENTERPRISE_NUMBER)
- final int enterpriseNumber,
+ @PluginAttribute(value = "enterpriseNumber", defaultString =
DEFAULT_ENTERPRISE_NUMBER)
+ final String enterpriseNumber,
Review Comment:
I think it is about time to deprecate this method and create a
`Rfc5424Layout.Builder` class. The builder should validate the
`enterpriseNumber` parameter in its `build()` method.
The signature of the method should remain the same.
##########
log4j-core/src/test/java/org/apache/logging/log4j/core/appender/TlsSyslogAppenderTest.java:
##########
@@ -94,7 +94,7 @@ private SyslogAppender createAppender() {
}
return SyslogAppender.createAppender("localhost", PORTNUM, "SSL",
sslConfiguration, 0, -1, true, "Test", true,
- false, Facility.LOCAL0, "Audit", 18060, true, "RequestContext",
null, null, includeNewLine, null,
+ false, Facility.LOCAL0, "Audit", "18060", true, "RequestContext",
null, null, includeNewLine, null,
Review Comment:
No need to modify the tests if backward compatibility is maintained.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
