You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/03/12 06:20:17 UTC
[GitHub] [druid] himanshug opened a new pull request #9507: optionally
disable all of hardcoded zookeeper use
himanshug opened a new pull request #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507
Paving Path Towards #9053
### Description
This patch adds a new configuration property `druid.zk.service.enabled=true/false, default = true` on all nodes to disable all of zookeeper activities that get setup even if user chooses to use HTTP based segment and task management. Some of those are....
- historical announcing itself as data server in zk
- historicals watching zk for segment load/drop requests
- historicals announcing segments in zk
- middle managers watching zk for task assignment requests
- middle managers doing task status updates in zk
(above set of things are required, so that curator based task/segment management continues to work in rolling deployment scenario and also could be interchanged with http based task/segment management at any time)
- HttpRemoteTaskRunner periodically doing cleanup of tasks from zk because MiddleManager continues to update the status in zk
- external discovery announcements done via `ServiceAnnouncer` to keep tranquility working
This property is undocumented for now till k8s based discovery extension PR shows up, that will have all the necessary documentation including setting `druid.zk.service.enabled=false` .
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
<hr>
##### Key changed/added classes in this PR
* `CliXXX`
* `XXXModule`
* And few others that directly/indirectly depended on `CuratorFramework`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on issue #9507: optionally disable
all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis commented on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-603005260
Some thoughts as I've been reviewing this (sorry I haven't finished yet):
Do you view this as an interim configuration, to allow your work to proceed on an alternative discovery mechanism, until we can decouple zookeeper specific code from all of the places that need to check this setting? or is the plan to leave it like this? So far I find it kind of ugly to have a setting like this due to all of the if/else branches it causes, but maybe there is some obvious reason I haven't got to yet on why we aren't adding some sort of `druid.discovery.type=zk|none` instead of this enable/disable setting. I'll keep reviewing, and try to finish up later tonight.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r396910915
##########
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:
Just thinking out loud, doesn't need to be addressed in this PR, it seems like maybe `LoadQueueTaskMaster` maybe needs some sort of peon factory that is set by config so that it doesn't have to care about individual implementations or curators and the like
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r396914173
##########
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:
It seems like this class is almost entirely to handle zk stuff, does it need to be bound and exist at all if zk is disabled?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on issue #9507: optionally disable all
of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
himanshug commented on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-603378061
@clintropolis thanks for clarification but I took the comments as in coming from a curious reviewer. In fact, with written communication, I always try to imagine a happy person speaking saying things.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on issue #9507: optionally disable
all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis commented on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-601936029
>@clintropolis can you please review/approve this one ?
Sorry yes, I have been meaning to take a look just haven't had much time to spare lately 😅I will try to have a look sometime today/this weekend.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on issue #9507: optionally disable all
of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
himanshug commented on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-601934321
@clintropolis can you please review/approve this one ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r396910109
##########
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:
This doesn't necessarily need to change in this PR, but it seems kind of leaky that this thing has a `CuratorFramework` at all, it seems like the load peon should provide this check so it can just be a no-op for non-zk. and then `DruidCoordinator` no longer needs a curator or zk paths I think?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on issue #9507: optionally disable
all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis commented on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-610053928
>I will try to get back to this today and finish up.
Oh man, this comment didn't age well, sorry I will try to get back to this PR this week.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r396907993
##########
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:
Hmm, is this actually true if still using zk segment loading? It was wrong before for the same reasons I think for http segment loading. Maybe this resource should accept either segment loader like `SegmentListerResource`, or we should have another http resource that we bind instead depending which mode 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on issue #9507: optionally disable all
of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
himanshug commented on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-601939257
@clintropolis thanks, I totally understand that :)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on issue #9507: optionally disable
all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis commented on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-604038197
>@clintropolis thanks for clarification but I took the comments as in coming from a curious reviewer. In fact, with written communication, I always try to imagine a happy person speaking things.
:+1: 😅
I will try to get back to this today and finish up.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
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<DataSegmentChangeRequest> 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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug edited a comment on issue #9507: optionally
disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
himanshug edited a comment on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-603378061
@clintropolis thanks for clarification but I took the comments as in coming from a curious reviewer. In fact, with written communication, I always try to imagine a happy person speaking things.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#discussion_r396909849
##########
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:
I haven't looked too closely yet, but I wonder if this would be better if this was marked as `@Nullable` and returned null instead of throwing the runtime exception, and shift the burden of validating that curator is available to the settings that do require it, such as inventory, segment loading, and task management? The other stuff might be able to be simplified a bit and not have to care about having the setting, and could probably avoid having some of the signature changes to use providers.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis edited a comment on issue #9507: optionally
disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
clintropolis edited a comment on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-603005260
Some thoughts as I've been reviewing this (sorry I haven't finished yet):
Do you view this as an interim configuration, to allow your work to proceed on an alternative discovery mechanism, until we can decouple zookeeper specific code from all of the places that need to check this setting? or is the plan to leave it like this? So far I find it kind of ugly to have a setting like this due to all of the if/else branches it causes, but maybe there is some obvious reason I haven't got to yet on why we aren't adding some sort of `druid.discovery.type=zk|none` instead of this enable/disable setting. I know some of the current HTTP modes are sort of leaky in terms of still doing zk stuff to support rolling update situations to transition settings, i would be in favor of breaking the current versions that support that, and adding some sort of composite or special mode to run both versions just for transition scenarios if that is the main driver to have the setting be this way.
I'll keep reviewing, and try to finish up later tonight.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on issue #9507: optionally disable all
of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
himanshug commented on issue #9507: optionally disable all of hardcoded zookeeper use
URL: https://github.com/apache/druid/pull/9507#issuecomment-603022760
@clintropolis Your concern is legit, I am not sure if you have seen https://groups.google.com/forum/#!msg/druid-development/tWnwPyL0Vk4/2uLwqgQiAAAJ and https://groups.google.com/forum/#!msg/druid-development/eIWDPfhpM_U/AzMRxSQGAgAJ , but please take a look at those. That will explain that extensible "discovery" to be implemented in extensions is "node/service discovery" and "leader election". Other zookeeper usage for segment/task management should be totally replaced by `HTTP` counterparts which I have been using since long time. All the disabling of non-extensible use of zookeeper introduced in this PR can be removed if/when we can delete zookeeper based segment/task management.
I hope with kubernetes discovery extension `HTTP` based segment/task management will get more adoption (as zookeeper based counterparts wouldn't work in that setting) and will eventually be deleted and so would all of the optionality introduced in this PR.
`druid.discovery.type=zk` does exist, it is `druid.discovery.type=curator` and is default which leads to usage of curator based discovery abstractions impl `CuratorDruidLeaderSelector`, `CurationDruidNodeAnnouncer` and `CuratorDruidNodeDiscoveryProvider`.
Next PR in this chain would have kubernetes based impl for all those i.e. `K8sDruidLeaderSelector`, `K8sDruidNodeAnnouncer` and `K8sDruidNodeDiscoveryProvider`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] himanshug commented on a change in pull request #9507:
optionally disable all of hardcoded zookeeper use
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org