You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "muralibasani (via GitHub)" <gi...@apache.org> on 2024/05/20 20:36:46 UTC
[PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
muralibasani opened a new pull request, #16005:
URL: https://github.com/apache/kafka/pull/16005
Details of the issue is defined in https://issues.apache.org/jira/browse/KAFKA-16790
In BrokerServer.scala, brokerMetadataPublishers are configured and when there are metadata updates remoteLogManager is not configured by then.
Ex : remoteLogManager.foreach(rlm => rlm.onLeadershipChange(partitionsBecomeLeader.asJava, partitionsBecomeFollower.asJava, topicIds)) in ReplicaManager is invoked after publishers are instantiated, and here rlm has relevant managers configured.
This change makes sure rlm is configured before the brokerMetadataPublishers initialization.
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [X] Verify design and implementation
- [X] Verify test coverage and CI build status
- [X] Verify documentation (including upgrade notes)
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2127157602
@nikramakrishnan somehow am not feeling confident to write a test based on the order of execution. Instead we should throw an exception (or stateChangeLogger.error) like mentioned below here, and write a unit test just for this class. With this approach, whenever there are metadata changes, and remote log storage is enabled. wdyt ?
> @clolov In ReplicaManager.scala, if REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP is enabled and if remoteLogManager is None do we want to throw any exception before the below line remoteLogManager.foreach(rlm => rlm.onLeadershipChange(l....
>
> then we can add a test easily
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2136932736
> @showuon can we still make this for 3.8.0, I think the deadline is today.
The feature freeze date is related to KIP, not bug fix. So no worries.
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #16005:
URL: https://github.com/apache/kafka/pull/16005#discussion_r1618546284
##########
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##########
@@ -340,6 +347,10 @@ public void onLeadershipChange(Set<Partition> partitionsBecomeLeader,
Set<TopicIdPartition> followerPartitions = filterPartitions(partitionsBecomeFollower)
.map(p -> new TopicIdPartition(topicIds.get(p.topic()), p.topicPartition())).collect(Collectors.toSet());
+ if (this.rlmConfig.enableRemoteStorageSystem() && !isRemoteLogManagerConfigured()) {
+ throw new KafkaException("RemoteLogManager is not configured when remote storage system is enabled");
+ }
+
Review Comment:
Agree, we can throw before deriving other vars. Moved.
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2138352697
@nikramakrishnan @showuon there are some test failures but not sure if related to ours ?
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "nikramakrishnan (via GitHub)" <gi...@apache.org>.
nikramakrishnan commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2129187298
> Instead we should throw an exception (or stateChangeLogger.error) like mentioned below here, and write a unit test just for this class
Do you mean we mock `onPartitionLeadershipChanges` to throw an exception if it is invoked without `configure` being called? We can take that approach as well.
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2129528563
@nikramakrishnan I have pushed a few changes. Please review.
We explicitly throw a KafkaException now if rlm is not configured and remote storage is enabled
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2138452239
Failed tests are unrelated.
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #16005:
URL: https://github.com/apache/kafka/pull/16005#discussion_r1618489890
##########
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala:
##########
@@ -1072,6 +1072,49 @@ class KRaftClusterTest {
}
}
+ @Test
+ def testCreateClusterAndCreateTopicWithRemoteLogManagerInstantiation(): Unit = {
+ val cluster = new KafkaClusterTestKit.Builder(
+ new TestKitNodes.Builder().
+ setNumBrokerNodes(3).
+ setNumControllerNodes(3).build())
Review Comment:
agree, we don't need. fixed
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #16005:
URL: https://github.com/apache/kafka/pull/16005#discussion_r1618530646
##########
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##########
@@ -340,6 +347,10 @@ public void onLeadershipChange(Set<Partition> partitionsBecomeLeader,
Set<TopicIdPartition> followerPartitions = filterPartitions(partitionsBecomeFollower)
.map(p -> new TopicIdPartition(topicIds.get(p.topic()), p.topicPartition())).collect(Collectors.toSet());
+ if (this.rlmConfig.enableRemoteStorageSystem() && !isRemoteLogManagerConfigured()) {
+ throw new KafkaException("RemoteLogManager is not configured when remote storage system is enabled");
+ }
+
Review Comment:
nit: Should we move this validation on the top of `onLeadershipChange` method?
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "nikramakrishnan (via GitHub)" <gi...@apache.org>.
nikramakrishnan commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2126713386
> @nikramakrishnan I think calling applyDelta on replicaManager wouldn't invoke configure on rlmm. Probably we should start mocking from BrokerServer.startup method, and create a topic, which invoke both configure and applyDelta wdyt ?
Agree to this. Maybe we could do something similar to what is being done in [DeleteTopicTest.createTestTopicAndCluster](https://github.com/apache/kafka/blob/trunk/core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala#L451)?
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2122679508
And I tried testing with export INCLUDE_TEST_JARS=true, but still notice class not found err. Am I missing any ?
--
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
Re: [PR] KAFKA-16790: Update RemoteLogManager configuration in broker server [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon merged PR #16005:
URL: https://github.com/apache/kafka/pull/16005
--
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
Re: [PR] KAFKA-16790: Update RemoteLogManager configuration in broker server [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2139031992
Thanks for persevering with this @muralibasani!
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "nikramakrishnan (via GitHub)" <gi...@apache.org>.
nikramakrishnan commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2137653319
Thanks for the changes @muralibasani, and sorry for the delay in getting back to this. These changes LGTM!
There seem to be failing tests related to this change though:
`testRemoteLogTaskUpdateRemoteLogSegmentMetadataAfterLogDirChanged` fails with:
```
org.apache.kafka.common.KafkaException: RemoteLogManager is not configured when remote storage system is enabled
```
Can you please check this?
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2121170470
@clolov would you like to take a look ? I have taken the first approach of configuring rlm before initializing publishers, instead of skipping if they don't exist. There can be multiple places where rlm configuration is required. So thought this is better.
Haven't added any explicit tests here, but do you suggest any ?
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #16005:
URL: https://github.com/apache/kafka/pull/16005#discussion_r1618489450
##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -4034,6 +4034,7 @@ class ReplicaManagerTest {
_ => Optional.of(mockLog),
(TopicPartition, Long) => {},
brokerTopicStats)
+ remoteLogManager.startup()
Review Comment:
Ah yes. fixed.
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2124671178
@clolov I tried to add
.setConfigProp(RemoteLogManagerConfig.REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP, true.toString)
.setConfigProp(RemoteLogManagerConfig.REMOTE_LOG_METADATA_MANAGER_CLASS_NAME_PROP,
"org.apache.kafka.server.log.remote.storage.NoOpRemoteLogMetadataManager")
.setConfigProp(RemoteLogManagerConfig.REMOTE_STORAGE_MANAGER_CLASS_NAME_PROP,
"org.apache.kafka.server.log.remote.storage.NoOpRemoteStorageManager")
to this test here
https://github.com/apache/kafka/blob/trunk/core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala#L163
What I noticed is remoteLogManager at https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/ReplicaManager.scala#L2737 is not null, as broker is fully started even with old code. This is confusing now.
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2136899900
@showuon can we still make this for 3.8.0, I think the deadline is today.
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2137624021
> LGTM! Will wait for CI build completes.
looks like ci is completed with warnings
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2122438149
This approach makes sense to me. How easy would it be to code up the test which I did manually in the JIRA ticket? In other words, without this change I would expect the test to fail with a NullPointerException (or an exception of your choice)
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2125492819
@nikramakrishnan
I think calling applyDelta on replicaManager wouldn't invoke configure on rlmm. Probably we should start mocking from BrokerServer.startup method, and create a topic, which invoke both configure and applyDelta
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2122602931
@clolov thanks for the tip.
Would it be better to write the test in KRaftClusterTest.scala or RemoteLogManagerTest or in a different file ?
As it's more of an integration test, where broker starts up, and if we mock it without rlm initialization, it would fail with NullPointer proobably.
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2137706649
> Thanks for the changes @muralibasani, and sorry for the delay in getting back to this. These changes LGTM!
>
> There seem to be failing tests related to this change though:
>
> `testRemoteLogTaskUpdateRemoteLogSegmentMetadataAfterLogDirChanged` fails with:
>
> ```
> org.apache.kafka.common.KafkaException: RemoteLogManager is not configured when remote storage system is enabled
> ```
>
> Can you please check this?
thanks. there seems to be a new test added yesterday. Updated 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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #16005:
URL: https://github.com/apache/kafka/pull/16005#discussion_r1618107975
##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -4034,6 +4034,7 @@ class ReplicaManagerTest {
_ => Optional.of(mockLog),
(TopicPartition, Long) => {},
brokerTopicStats)
+ remoteLogManager.startup()
Review Comment:
nit: indent is wrong.
##########
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala:
##########
@@ -1072,6 +1072,49 @@ class KRaftClusterTest {
}
}
+ @Test
+ def testCreateClusterAndCreateTopicWithRemoteLogManagerInstantiation(): Unit = {
+ val cluster = new KafkaClusterTestKit.Builder(
+ new TestKitNodes.Builder().
+ setNumBrokerNodes(3).
+ setNumControllerNodes(3).build())
Review Comment:
nit: 1 broker + 1 controller should be enough for this test scenario, right?
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2124458899
@clolov
In ReplicaManager.scala,
if REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP is enabled and if remoteLogManager is None
do we want to throw any exception before the below line
remoteLogManager.foreach(rlm => rlm.onLeadershipChange(l....
--
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
Re: [PR] Update RemoteLogManager configuration in broker server - KAFKA-16790 [kafka]
Posted by "nikramakrishnan (via GitHub)" <gi...@apache.org>.
nikramakrishnan commented on PR #16005:
URL: https://github.com/apache/kafka/pull/16005#issuecomment-2125001875
Hi @muralibasani, thanks for the PR!
As I understand it, the issue here is that `rlm.startup()` (which internally calls `remoteLogMetadataManager.configure`) is not called first when there is metadata to catch up on during broker startup. This means that RemoteLogManager may be non-null, but `applyDelta` ends up calling `rlm.onLeadershipChange` (which calls `remoteLogMetadataManager.onPartitionLeadershipChanges`) before `configure` is invoked on `remoteLogMetadataManager`.
A possible way to test this could be to mock `configure` and `onPartitionLeadershipChanges` in `RemoteLogMetadataManager`, then call `applyDelta` with some leadership changes, and assert that configure was called on the mock before `onPartitionLeadershipChanges`.
--
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