Re: [PR] Fix potential NPE in broker dynamic config syncer (druid)

2026-03-11 Thread via GitHub 


jtuglu1 merged PR #19140:
URL: https://github.com/apache/druid/pull/19140


-- 
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]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix potential NPE in broker dynamic config syncer (druid)

2026-03-11 Thread via GitHub 


jtuglu1 commented on code in PR #19140:
URL: https://github.com/apache/druid/pull/19140#discussion_r2922304172


##
server/src/main/java/org/apache/druid/server/http/BaseDynamicConfigSyncer.java:
##
@@ -180,6 +180,10 @@ private void pushConfigToBrokerNode(DiscoveryDruidNode 
broker)
   {
 final Stopwatch stopwatch = Stopwatch.createStarted();
 final ServiceLocation brokerLocation = 
convertDiscoveryNodeToServiceLocation(broker);
+if (brokerLocation == null) {
+  log.warn("Skipping broker sync - could not convert to ServiceLocation: 
[%s]", broker);

Review Comment:
   Updated



-- 
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]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix potential NPE in broker dynamic config syncer (druid)

2026-03-11 Thread via GitHub 


kfaraz commented on code in PR #19140:
URL: https://github.com/apache/druid/pull/19140#discussion_r2922290186


##
server/src/main/java/org/apache/druid/server/http/BaseDynamicConfigSyncer.java:
##
@@ -180,6 +180,10 @@ private void pushConfigToBrokerNode(DiscoveryDruidNode 
broker)
   {
 final Stopwatch stopwatch = Stopwatch.createStarted();
 final ServiceLocation brokerLocation = 
convertDiscoveryNodeToServiceLocation(broker);
+if (brokerLocation == null) {
+  log.warn("Skipping broker sync - could not convert to ServiceLocation: 
[%s]", broker);

Review Comment:
   Nit: Should this log be moved to the `convertDiscoveryNode` since it has 
more information about why the service location could not be determined (e.g. 
`DruidNode` is null)?
   ```suggestion
 log.warn("Skipping config sync for broker[%s] as its ServiceLocation 
could not be determined.", broker);
   ```



-- 
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]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Fix potential NPE in broker dynamic config syncer (druid)

2026-03-11 Thread via GitHub 


github-advanced-security[bot] commented on code in PR #19140:
URL: https://github.com/apache/druid/pull/19140#discussion_r2922149826


##
server/src/test/java/org/apache/druid/server/http/CoordinatorDynamicConfigSyncerTest.java:
##
@@ -103,4 +103,30 @@
 .jsonContent(DefaultObjectMapper.INSTANCE, config);
 verify(serviceClient).asyncRequest(eq(requestBuilder), 
ArgumentMatchers.any());
   }
+
+  @Test
+  public void testSync_whenDruidNode_isNull()
+  {
+CoordinatorDynamicConfig config = CoordinatorDynamicConfig
+.builder()
+.withMaxSegmentsToMove(105)
+.withReplicantLifetime(500)
+.withReplicationThrottleLimit(5)
+.build();
+
+doReturn(config).when(coordinatorConfigManager).getCurrentDynamicConfig();
+List nodes = List.of(
+new DiscoveryDruidNode(
+null,
+NodeRole.BROKER,
+null,
+null
+)
+);
+doReturn(nodes).when(druidNodeDiscovery).getAllNodes();
+
+target.broadcastConfigToBrokers();
+RequestBuilder requestBuilder = new RequestBuilder(HttpMethod.POST, 
"/druid-internal/v1/config/coordinator")
+.jsonContent(DefaultObjectMapper.INSTANCE, config);

Review Comment:
   ## Unread local variable
   
   Variable 'RequestBuilder requestBuilder' is never read.
   
   [Show more 
details](https://github.com/apache/druid/security/code-scanning/10883)



-- 
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]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]