You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/03/27 14:48:11 UTC
[GitHub] [lucene-solr] madrob opened a new pull request #1384: Remove
CurrentCoreDescriptorProvider
madrob opened a new pull request #1384: Remove CurrentCoreDescriptorProvider
URL: https://github.com/apache/lucene-solr/pull/1384
Replace `CurrentCoreDescriptorProvider` with a functional interface so that it is easier to construct since all implementations in our code base were anonymous classes anyway. Added Javadocs explaining the usage instead of relying on class name to convey information.
----------------------------------------------------------------
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: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1384:
Remove CurrentCoreDescriptorProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1384: Remove CurrentCoreDescriptorProvider
URL: https://github.com/apache/lucene-solr/pull/1384#discussion_r399368766
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -469,7 +477,7 @@ public boolean isClosed() {
if (cc != null) cc.securityNodeChanged();
});
- init(registerOnReconnect);
+ init();
Review comment:
Just looking in GitHub review here but did you omit this param because it's already in scope?
----------------------------------------------------------------
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: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #1384:
Remove CurrentCoreDescriptorProvider
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1384: Remove CurrentCoreDescriptorProvider
URL: https://github.com/apache/lucene-solr/pull/1384#discussion_r399370228
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -469,7 +477,7 @@ public boolean isClosed() {
if (cc != null) cc.securityNodeChanged();
});
- init(registerOnReconnect);
+ init();
Review comment:
It was unused in the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1384:
Remove CurrentCoreDescriptorProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1384: Remove CurrentCoreDescriptorProvider
URL: https://github.com/apache/lucene-solr/pull/1384#discussion_r399368319
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -287,7 +288,14 @@ public Object call() throws Exception {
}
}
- public ZkController(final CoreContainer cc, String zkServerAddress, int zkClientConnectTimeout, CloudConfig cloudConfig, final CurrentCoreDescriptorProvider registerOnReconnect)
+ /**
+ * @param cc Core container associated with this controller. cannot be null.
+ * @param zkServerAddress where to connect to the zk server
+ * @param zkClientConnectTimeout timeout in ms
+ * @param cloudConfig configuration for this controller. TODO: possibly redundant with CoreContainer
+ * @param registerOnReconnect a provider of the current core descriptors. used to know what to reregister after a reconnect
Review comment:
I wonder if "registerOnReconnect" is not appropriate for the name. It's named how it's being used but not for what it is. Imagine how silly if all things were named with such an approach. Perhaps simply "descriptorProvider". Any way, leave or change as you wish.
----------------------------------------------------------------
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: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob merged pull request #1384: Remove
CurrentCoreDescriptorProvider
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #1384: Remove CurrentCoreDescriptorProvider
URL: https://github.com/apache/lucene-solr/pull/1384
----------------------------------------------------------------
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: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on issue #1384: Remove
CurrentCoreDescriptorProvider
Posted by GitBox <gi...@apache.org>.
madrob commented on issue #1384: Remove CurrentCoreDescriptorProvider
URL: https://github.com/apache/lucene-solr/pull/1384#issuecomment-605099729
Is ZkController public API? Do I need a JIRA and CHANGES entry for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1384:
Remove CurrentCoreDescriptorProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1384: Remove CurrentCoreDescriptorProvider
URL: https://github.com/apache/lucene-solr/pull/1384#discussion_r399365014
##########
File path: solr/core/src/java/org/apache/solr/core/ZkContainer.java
##########
@@ -112,20 +111,14 @@ public void initZooKeeper(final CoreContainer cc, CloudConfig config) {
throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR,
"A chroot was specified in ZkHost but the znode doesn't exist. " + zookeeperHost);
}
- ZkController zkController = new ZkController(cc, zookeeperHost, zkClientConnectTimeout, config,
- new CurrentCoreDescriptorProvider() {
-
- @Override
- public List<CoreDescriptor> getCurrentDescriptors() {
- List<CoreDescriptor> descriptors = new ArrayList<>(
- cc.getLoadedCoreNames().size());
- Collection<SolrCore> cores = cc.getCores();
- for (SolrCore core : cores) {
- descriptors.add(core.getCoreDescriptor());
- }
- return descriptors;
- }
- });
+ ZkController zkController = new ZkController(cc, zookeeperHost, zkClientConnectTimeout, config, () -> {
Review comment:
I'd prefer that you declare that last parameter as a local typed variable and then pass it in. This makes it clearer what it is that is being passed to ZkController's c'tor.
----------------------------------------------------------------
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: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org