[GitHub] [druid] himanshug commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
himanshug commented on a change in pull request #9507: optionally disable all
of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r397320944
##
File path:
server/src/main/java/org/apache/druid/server/coordination/BatchDataSegmentAnnouncer.java
##
@@ -99,13 +107,28 @@ public BatchDataSegmentAnnouncer(
return rv;
};
-if (this.config.isSkipSegmentAnnouncementOnZk()) {
+isSkipSegmentAnnouncementOnZk = !zkEnablementConfig.isEnabled() ||
config.isSkipSegmentAnnouncementOnZk();
Review comment:
this is also calling methods in
`private final ChangeRequestHistory changes = new
ChangeRequestHistory<>();`
which is used by the http endpoint for segment sync.
However, yeah it takes more work to announce stuff in zk so it looks like
that this class is primarily doing that whereas it is actually supporting both.
If/When we are able to delete zk based segment management this class would
shrink significantly.
Also, unrelated, it could possibly be refactored to separate two things it
is doing so that things are more clear.
However, you know my preference, just delete zk stuff :)
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
himanshug commented on a change in pull request #9507: optionally disable all
of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r397317523
##
File path:
server/src/main/java/org/apache/druid/server/coordinator/LoadQueueTaskMaster.java
##
@@ -67,7 +68,7 @@ public LoadQueuePeon giveMePeon(ImmutableDruidServer server)
return new HttpLoadQueuePeon(server.getURL(), jsonMapper, httpClient,
config, peonExec, callbackExec);
} else {
return new CuratorLoadQueuePeon(
- curator,
+ curatorFrameworkProvider.get(),
Review comment:
could be, but you know my preference by now :)
I wish people would migrate to using `HTTP` segment management and that
would remain the only way and this would be deleted.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
himanshug commented on a change in pull request #9507: optionally disable all
of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r397316647
##
File path:
server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##
@@ -443,7 +455,7 @@ public void moveSegment(
() -> {
try {
if
(serverInventoryView.isSegmentLoadedByServer(toServer.getName(), segment) &&
-curator.checkExists().forPath(toLoadQueueSegPath) == null
&&
+(curator == null ||
curator.checkExists().forPath(toLoadQueueSegPath) == null) &&
Review comment:
yeah, I hope zk based segment management will just go away and this will go
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
himanshug commented on a change in pull request #9507: optionally disable all
of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r397315538
##
File path: server/src/main/java/org/apache/druid/curator/CuratorModule.java
##
@@ -63,15 +63,20 @@
@Override
public void configure(Binder binder)
{
+JsonConfigProvider.bind(binder, CURATOR_CONFIG_PREFIX,
ZkEnablementConfig.class);
JsonConfigProvider.bind(binder, CURATOR_CONFIG_PREFIX,
CuratorConfig.class);
JsonConfigProvider.bind(binder, EXHIBITOR_CONFIG_PREFIX,
ExhibitorConfig.class);
}
@Provides
@LazySingleton
@SuppressForbidden(reason = "System#err")
- public CuratorFramework makeCurator(CuratorConfig config, EnsembleProvider
ensembleProvider, Lifecycle lifecycle)
+ public CuratorFramework makeCurator(ZkEnablementConfig zkEnablementConfig,
CuratorConfig config, EnsembleProvider ensembleProvider, Lifecycle lifecycle)
Review comment:
Main reason for failing loudly here is so that if I forgot to disable zk in
some code path, then this method would immediately fail on node start with
guice injection errors leading to quick discovery of exactly what is missed.
this helped me catch quite a few places that I missed.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
himanshug commented on a change in pull request #9507: optionally disable all
of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r397314006
##
File path:
server/src/main/java/org/apache/druid/server/http/HistoricalResource.java
##
@@ -50,14 +50,14 @@ public HistoricalResource(
@Produces(MediaType.APPLICATION_JSON)
public Response getLoadStatus()
{
-return Response.ok(ImmutableMap.of("cacheInitialized",
coordinator.isStarted())).build();
+return Response.ok(ImmutableMap.of("cacheInitialized",
segmentLoadDropHandler.isStarted())).build();
}
@GET
@Path("/readiness")
public Response getReadiness()
{
-if (coordinator.isStarted()) {
+if (segmentLoadDropHandler.isStarted()) {
Review comment:
primary thing checked by this endpoint is that historical startup
sequence has finished loading/announcing segments that it already had on
disk which is often a time consuming activity. that is still ensured by
`segmentLoadDropHandler.isStarted()`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
