You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "cmccabe (via GitHub)" <gi...@apache.org> on 2023/05/10 18:02:38 UTC
[GitHub] [kafka] cmccabe opened a new pull request, #13703: MINOR: Standardize controller log4j output for replaying records
cmccabe opened a new pull request, #13703:
URL: https://github.com/apache/kafka/pull/13703
Standardize controller log4j output for replaying important records. The log message should include word "replayed" to make it clear that this is a record replay. Log the replay of records for ACLs, client quotas, and producer IDs, which were previously not logged. Also fix a case where we weren't logging changes to broker registrations.
AclControlManager, ClientQuotaControlManager, and ProducerIdControlManager didn't previously have a log4j logger object, so this PR adds one. It also converts them to using Builder objects. This makes junit tests more readable because we don't need to specify paramaters where the test can use the default (like LogContexts).
Throw an exception in replay if we get another TopicRecord for a topic which already exists.
--
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] cmccabe commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1262846403
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -377,7 +377,19 @@ private ReplicationControlManager(
}
public void replay(TopicRecord record) {
- topicsByName.put(record.name(), record.topicId());
+ Uuid existingUuid = topicsByName.put(record.name(), record.topicId());
+ if (existingUuid != null) {
+ // We don't currently support sending a second TopicRecord for the same topic name...
+ // unless, of course, there is a RemoveTopicRecord in between.
+ if (existingUuid.equals(record.topicId())) {
+ throw new RuntimeException("Found duplicate TopicRecord for " + record.name() +
+ " with topic ID " + record.topicId());
+ } else {
+ throw new RuntimeException("Found duplicate TopicRecord for " + record.name() +
+ " with a different ID than before. Previous ID was " + existingUuid +
+ " and new ID is " + record.topicId());
+ }
+ }
Review Comment:
The RuntimeException is handled in `handleEventException`
--
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] cmccabe commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1262848304
##########
metadata/src/test/java/org/apache/kafka/controller/MockAclControlManager.java:
##########
@@ -33,7 +33,7 @@
public class MockAclControlManager extends AclControlManager {
public MockAclControlManager(LogContext logContext,
Optional<ClusterMetadataAuthorizer> authorizer) {
- super(new SnapshotRegistry(logContext), authorizer);
+ super(new LogContext(), new SnapshotRegistry(logContext), authorizer);
Review Comment:
fixed, thanks.
--
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] divijvaidya commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1204039964
##########
metadata/src/main/java/org/apache/kafka/controller/AclControlManager.java:
##########
@@ -64,12 +66,44 @@
* completed, which is another reason the prepare / complete callbacks are needed.
*/
public class AclControlManager {
+ static class Builder {
+ private LogContext logContext = null;
+ private SnapshotRegistry snapshotRegistry = null;
+ private Optional<ClusterMetadataAuthorizer> authorizer = Optional.empty();
+
+ Builder setLogContext(LogContext logContext) {
+ this.logContext = logContext;
+ return this;
+ }
+
+ Builder setSnapshotRegistry(SnapshotRegistry snapshotRegistry) {
+ this.snapshotRegistry = snapshotRegistry;
+ return this;
+ }
+
+ Builder setClusterMetadataAuthorizer(Optional<ClusterMetadataAuthorizer> authorizer) {
+ this.authorizer = authorizer;
+ return this;
+ }
+
+ AclControlManager build() {
+ if (logContext == null) logContext = new LogContext();
Review Comment:
Do you think it would be useful to add a LogPrefix here (and other places changed in this PR) such as
```
LogContext("[AclControlManager id=nodeId")
```
--
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] divijvaidya commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1219188642
##########
metadata/src/main/java/org/apache/kafka/controller/AclControlManager.java:
##########
@@ -64,12 +66,44 @@
* completed, which is another reason the prepare / complete callbacks are needed.
*/
public class AclControlManager {
+ static class Builder {
+ private LogContext logContext = null;
+ private SnapshotRegistry snapshotRegistry = null;
+ private Optional<ClusterMetadataAuthorizer> authorizer = Optional.empty();
+
+ Builder setLogContext(LogContext logContext) {
+ this.logContext = logContext;
+ return this;
+ }
+
+ Builder setSnapshotRegistry(SnapshotRegistry snapshotRegistry) {
+ this.snapshotRegistry = snapshotRegistry;
+ return this;
+ }
+
+ Builder setClusterMetadataAuthorizer(Optional<ClusterMetadataAuthorizer> authorizer) {
+ this.authorizer = authorizer;
+ return this;
+ }
+
+ AclControlManager build() {
+ if (logContext == null) logContext = new LogContext();
Review Comment:
ok, sounds good.
--
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] rondagostino commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1218423667
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -377,7 +377,19 @@ private ReplicationControlManager(
}
public void replay(TopicRecord record) {
- topicsByName.put(record.name(), record.topicId());
+ Uuid existingUuid = topicsByName.put(record.name(), record.topicId());
+ if (existingUuid != null) {
+ // We don't currently support sending a second TopicRecord for the same topic name...
+ // unless, of course, there is a RemoveTopicRecord in between.
+ if (existingUuid.equals(record.topicId())) {
+ throw new RuntimeException("Found duplicate TopicRecord for " + record.name() +
+ " with topic ID " + record.topicId());
Review Comment:
nit: Noting that this first clause is explicitly rejecting the possibility of an idempotent replay of the same record a second time. This makes sense, as it is unexpected if it should happen.
It might be good to call this out explicitly in a comment.
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -473,8 +488,8 @@ public void replay(PartitionChangeRecord record) {
if (record.removingReplicas() != null || record.addingReplicas() != null) {
log.info("Replayed partition assignment change {} for topic {}", record, topicInfo.name);
- } else if (log.isTraceEnabled()) {
- log.trace("Replayed partition change {} for topic {}", record, topicInfo.name);
+ } else if (log.isDebugEnabled()) {
Review Comment:
+1
##########
metadata/src/test/java/org/apache/kafka/controller/MockAclControlManager.java:
##########
@@ -33,7 +33,7 @@
public class MockAclControlManager extends AclControlManager {
public MockAclControlManager(LogContext logContext,
Optional<ClusterMetadataAuthorizer> authorizer) {
- super(new SnapshotRegistry(logContext), authorizer);
+ super(new LogContext(), new SnapshotRegistry(logContext), authorizer);
Review Comment:
Seems like it should be `s/new LogContext()/logContext/`?
##########
metadata/src/main/java/org/apache/kafka/controller/AclControlManager.java:
##########
@@ -64,12 +66,44 @@
* completed, which is another reason the prepare / complete callbacks are needed.
*/
public class AclControlManager {
+ static class Builder {
+ private LogContext logContext = null;
+ private SnapshotRegistry snapshotRegistry = null;
+ private Optional<ClusterMetadataAuthorizer> authorizer = Optional.empty();
+
+ Builder setLogContext(LogContext logContext) {
+ this.logContext = logContext;
+ return this;
+ }
+
+ Builder setSnapshotRegistry(SnapshotRegistry snapshotRegistry) {
+ this.snapshotRegistry = snapshotRegistry;
+ return this;
+ }
+
+ Builder setClusterMetadataAuthorizer(Optional<ClusterMetadataAuthorizer> authorizer) {
+ this.authorizer = authorizer;
+ return this;
+ }
+
+ AclControlManager build() {
+ if (logContext == null) logContext = new LogContext();
Review Comment:
I agree this is a worthwhile question to consider. I do note that the builders that already exist don't set anything special. For example, from `ConfigurationControlManager.Builder.build()`:
```
if (logContext == null) logContext = new LogContext();
```
And all of these builder log contexts do get explicitly set in QuorumController (including the new ones via this PR), so the default will never be used.
So my guess is the answer is no, it isn't worth doing. Colin, WDYT?
--
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] cmccabe commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1218713351
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -377,7 +377,19 @@ private ReplicationControlManager(
}
public void replay(TopicRecord record) {
- topicsByName.put(record.name(), record.topicId());
+ Uuid existingUuid = topicsByName.put(record.name(), record.topicId());
+ if (existingUuid != null) {
+ // We don't currently support sending a second TopicRecord for the same topic name...
+ // unless, of course, there is a RemoveTopicRecord in between.
+ if (existingUuid.equals(record.topicId())) {
+ throw new RuntimeException("Found duplicate TopicRecord for " + record.name() +
+ " with topic ID " + record.topicId());
Review Comment:
The comment says:
```
// We don't currently support sending a second TopicRecord for the same topic name...
// unless, of course, there is a RemoveTopicRecord in between.```
which I think does cover the scenario you were thinking of (let me know if this makes sense)
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -377,7 +377,19 @@ private ReplicationControlManager(
}
public void replay(TopicRecord record) {
- topicsByName.put(record.name(), record.topicId());
+ Uuid existingUuid = topicsByName.put(record.name(), record.topicId());
+ if (existingUuid != null) {
+ // We don't currently support sending a second TopicRecord for the same topic name...
+ // unless, of course, there is a RemoveTopicRecord in between.
+ if (existingUuid.equals(record.topicId())) {
+ throw new RuntimeException("Found duplicate TopicRecord for " + record.name() +
+ " with topic ID " + record.topicId());
Review Comment:
The comment says:
```
// We don't currently support sending a second TopicRecord for the same topic name...
// unless, of course, there is a RemoveTopicRecord in between.
```
which I think does cover the scenario you were thinking of (let me know if this makes sense)
--
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] ijuma commented on pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #13703:
URL: https://github.com/apache/kafka/pull/13703#issuecomment-1636638363
+1 @jolshan - is there a reason why people are not checking the build result? Particularly when it doesn't even compile.
--
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] cmccabe commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1218710516
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -473,8 +488,8 @@ public void replay(PartitionChangeRecord record) {
if (record.removingReplicas() != null || record.addingReplicas() != null) {
log.info("Replayed partition assignment change {} for topic {}", record, topicInfo.name);
- } else if (log.isTraceEnabled()) {
- log.trace("Replayed partition change {} for topic {}", record, topicInfo.name);
+ } else if (log.isDebugEnabled()) {
Review Comment:
good point. I'll remove 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] cmccabe commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1218712388
##########
metadata/src/main/java/org/apache/kafka/controller/AclControlManager.java:
##########
@@ -64,12 +66,44 @@
* completed, which is another reason the prepare / complete callbacks are needed.
*/
public class AclControlManager {
+ static class Builder {
+ private LogContext logContext = null;
+ private SnapshotRegistry snapshotRegistry = null;
+ private Optional<ClusterMetadataAuthorizer> authorizer = Optional.empty();
+
+ Builder setLogContext(LogContext logContext) {
+ this.logContext = logContext;
+ return this;
+ }
+
+ Builder setSnapshotRegistry(SnapshotRegistry snapshotRegistry) {
+ this.snapshotRegistry = snapshotRegistry;
+ return this;
+ }
+
+ Builder setClusterMetadataAuthorizer(Optional<ClusterMetadataAuthorizer> authorizer) {
+ this.authorizer = authorizer;
+ return this;
+ }
+
+ AclControlManager build() {
+ if (logContext == null) logContext = new LogContext();
Review Comment:
@rondagostino is correct that the defaults here do not get used except in unit tests.
In general we want `[QuorumController id=1] ` to appear before every quorum controller log message. The class name (AclControlManager or whatever) actually gets logged separately at the end of the line, when using the typical log4j setup.
--
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] cmccabe commented on pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #13703:
URL: https://github.com/apache/kafka/pull/13703#issuecomment-1634618543
> And all of these builder log contexts do get explicitly set in QuorumController (including the new ones via this PR), so the default will never be used. So my guess is the answer is no, it isn't worth doing. Colin, WDYT?
Well, the JUnit tests don't set the log context. The builder is useful for avoiding clutter in these tests.
In general most of the controller manager objects should have builders. It just avoids having a huge amount of boilerplate in the tests (like dummy LogContext, SnapshotRegistry, objects etc.) It also makes it bearable to add new constructor parameters without updating every since test call site.
--
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] cmccabe commented on pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #13703:
URL: https://github.com/apache/kafka/pull/13703#issuecomment-1637136736
As I recall, we did check the Jenkins result, it's just that it was an old and stale build result. I should have re-run the build. Sorry.
--
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] divijvaidya commented on a diff in pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13703:
URL: https://github.com/apache/kafka/pull/13703#discussion_r1203778428
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -473,8 +488,8 @@ public void replay(PartitionChangeRecord record) {
if (record.removingReplicas() != null || record.addingReplicas() != null) {
log.info("Replayed partition assignment change {} for topic {}", record, topicInfo.name);
- } else if (log.isTraceEnabled()) {
- log.trace("Replayed partition change {} for topic {}", record, topicInfo.name);
+ } else if (log.isDebugEnabled()) {
Review Comment:
Do we really need this check? My understanding is that the parameterised messages (which we are using here) removes the requirement of this check. see: https://www.slf4j.org/faq.html#logging_performance
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -377,7 +377,19 @@ private ReplicationControlManager(
}
public void replay(TopicRecord record) {
- topicsByName.put(record.name(), record.topicId());
+ Uuid existingUuid = topicsByName.put(record.name(), record.topicId());
+ if (existingUuid != null) {
+ // We don't currently support sending a second TopicRecord for the same topic name...
+ // unless, of course, there is a RemoveTopicRecord in between.
+ if (existingUuid.equals(record.topicId())) {
+ throw new RuntimeException("Found duplicate TopicRecord for " + record.name() +
+ " with topic ID " + record.topicId());
+ } else {
+ throw new RuntimeException("Found duplicate TopicRecord for " + record.name() +
+ " with a different ID than before. Previous ID was " + existingUuid +
+ " and new ID is " + record.topicId());
+ }
+ }
Review Comment:
1. This should perhaps go in a separate PR? It would help keeping commits as separate (thus helping in faster rollback if required)
2. I am curious to understand where the RuntimeException gets logged (probably handled by event queue?)
--
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] cmccabe merged pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe merged PR #13703:
URL: https://github.com/apache/kafka/pull/13703
--
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] jolshan commented on pull request #13703: MINOR: Standardize controller log4j output for replaying records
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on PR #13703:
URL: https://github.com/apache/kafka/pull/13703#issuecomment-1636124780
Guys -- we really need to check builds before merging.
--
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