ndimiduk commented on code in PR #5770:
URL: https://github.com/apache/hbase/pull/5770#discussion_r1542616380
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java:
##########
@@ -17,27 +17,75 @@
*/
package org.apache.hadoop.hbase.client;
-import static
org.apache.hadoop.hbase.HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY;
-
+import java.io.IOException;
+import java.net.URI;
+import java.util.ServiceLoader;
+import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.security.User;
import org.apache.hadoop.hbase.util.ReflectionUtils;
import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap;
/**
- * Factory class to get the instance of configured connection registry.
+ * The entry point for creating a {@link ConnectionRegistry}.
*/
@InterfaceAudience.Private
final class ConnectionRegistryFactory {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConnectionRegistryFactory.class);
+
+ private static final ImmutableMap<String, ConnectionRegistryCreator>
CREATORS;
+ static {
+ ImmutableMap.Builder<String, ConnectionRegistryCreator> builder =
ImmutableMap.builder();
+ for (ConnectionRegistryCreator factory :
ServiceLoader.load(ConnectionRegistryCreator.class)) {
+ builder.put(factory.protocol(), factory);
+ }
+ CREATORS = builder.build();
+ }
+
private ConnectionRegistryFactory() {
}
- /** Returns The connection registry implementation to use. */
- static ConnectionRegistry getRegistry(Configuration conf, User user) {
+ /**
+ * Returns the connection registry implementation to use, for the given
connection url
+ * {@code uri}.
+ * <p/>
+ * We use {@link ServiceLoader} to load different implementations, and use
the scheme of the given
+ * {@code uri} to select. And if there is no protocol specified, or we can
not find a
+ * {@link ConnectionRegistryCreator} implementation for the given scheme, we
will fallback to use
+ * the old way to create the {@link ConnectionRegistry}. Notice that, if
fallback happens, the
+ * specified connection url {@code uri} will not take effect, we will load
all the related
+ * configurations from the given Configuration instance {@code conf}
+ */
+ static ConnectionRegistry create(URI uri, Configuration conf, User user)
throws IOException {
+ if (StringUtils.isBlank(uri.getScheme())) {
+ LOG.warn("No scheme specified for {}, fallback to use old way", uri);
Review Comment:
I think that we can handle this better.
In the case that the URI has no protocol part but does include the host
part, we should fall back to whatever is the default ConnectionRegistry
implementation. In this case, maybe a warning log is a good idea, but maybe
it's fine to simply permit this as a standard means of configuring the client.
In the case that the URI has no protocol part and no host part, then there's
nothing we can do with this. We should throw an exception. Maybe that will
happen anyway -- my recollection is that Java's URI parser is pretty strict but
I don't know which parts of a URI are required by the RFC.
Either way, if we're going to log something, we should be more precise than
"the old way". We should tell the operator what is about to happen.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java:
##########
@@ -17,27 +17,75 @@
*/
package org.apache.hadoop.hbase.client;
-import static
org.apache.hadoop.hbase.HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY;
-
+import java.io.IOException;
+import java.net.URI;
+import java.util.ServiceLoader;
+import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.security.User;
import org.apache.hadoop.hbase.util.ReflectionUtils;
import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap;
/**
- * Factory class to get the instance of configured connection registry.
+ * The entry point for creating a {@link ConnectionRegistry}.
*/
@InterfaceAudience.Private
final class ConnectionRegistryFactory {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConnectionRegistryFactory.class);
+
+ private static final ImmutableMap<String, ConnectionRegistryCreator>
CREATORS;
+ static {
+ ImmutableMap.Builder<String, ConnectionRegistryCreator> builder =
ImmutableMap.builder();
+ for (ConnectionRegistryCreator factory :
ServiceLoader.load(ConnectionRegistryCreator.class)) {
+ builder.put(factory.protocol(), factory);
+ }
+ CREATORS = builder.build();
+ }
+
private ConnectionRegistryFactory() {
}
- /** Returns The connection registry implementation to use. */
- static ConnectionRegistry getRegistry(Configuration conf, User user) {
+ /**
+ * Returns the connection registry implementation to use, for the given
connection url
+ * {@code uri}.
+ * <p/>
+ * We use {@link ServiceLoader} to load different implementations, and use
the scheme of the given
+ * {@code uri} to select. And if there is no protocol specified, or we can
not find a
+ * {@link ConnectionRegistryCreator} implementation for the given scheme, we
will fallback to use
+ * the old way to create the {@link ConnectionRegistry}. Notice that, if
fallback happens, the
+ * specified connection url {@code uri} will not take effect, we will load
all the related
+ * configurations from the given Configuration instance {@code conf}
+ */
+ static ConnectionRegistry create(URI uri, Configuration conf, User user)
throws IOException {
Review Comment:
FYI, I did a search on URI vs. URL in Java and I conclude that URI is the
correct object type for our use-case.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java:
##########
@@ -17,27 +17,75 @@
*/
package org.apache.hadoop.hbase.client;
-import static
org.apache.hadoop.hbase.HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY;
-
+import java.io.IOException;
+import java.net.URI;
+import java.util.ServiceLoader;
+import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.security.User;
import org.apache.hadoop.hbase.util.ReflectionUtils;
import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap;
/**
- * Factory class to get the instance of configured connection registry.
+ * The entry point for creating a {@link ConnectionRegistry}.
*/
@InterfaceAudience.Private
final class ConnectionRegistryFactory {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConnectionRegistryFactory.class);
+
+ private static final ImmutableMap<String, ConnectionRegistryCreator>
CREATORS;
+ static {
+ ImmutableMap.Builder<String, ConnectionRegistryCreator> builder =
ImmutableMap.builder();
+ for (ConnectionRegistryCreator factory :
ServiceLoader.load(ConnectionRegistryCreator.class)) {
+ builder.put(factory.protocol(), factory);
Review Comment:
Wikipedia says that schemes should be case-insensitive. We should normalize
these before adding them to the map.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryCreator.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.client;
+
+import java.io.IOException;
+import java.net.URI;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.security.User;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * For creating different {@link ConnectionRegistry} implementation.
+ */
[email protected]
+public interface ConnectionRegistryCreator {
Review Comment:
Should this be a `ConnectionRegistryFactory` instead? I looks like a factory
to me.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestBasicReadWriteWithDifferentConnectionRegistries.java:
##########
@@ -0,0 +1,177 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.client;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertFalse;
+
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.TableNameTestRule;
+import org.apache.hadoop.hbase.testclassification.ClientTests;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Test basic read write operation with different {@link ConnectionRegistry}
implementations.
+ */
+@RunWith(Parameterized.class)
+@Category({ MediumTests.class, ClientTests.class })
+public class TestBasicReadWriteWithDifferentConnectionRegistries {
Review Comment:
Nice test.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java:
##########
@@ -17,27 +17,75 @@
*/
package org.apache.hadoop.hbase.client;
-import static
org.apache.hadoop.hbase.HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY;
-
+import java.io.IOException;
+import java.net.URI;
+import java.util.ServiceLoader;
+import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.security.User;
import org.apache.hadoop.hbase.util.ReflectionUtils;
import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap;
/**
- * Factory class to get the instance of configured connection registry.
+ * The entry point for creating a {@link ConnectionRegistry}.
*/
@InterfaceAudience.Private
final class ConnectionRegistryFactory {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConnectionRegistryFactory.class);
+
+ private static final ImmutableMap<String, ConnectionRegistryCreator>
CREATORS;
+ static {
+ ImmutableMap.Builder<String, ConnectionRegistryCreator> builder =
ImmutableMap.builder();
+ for (ConnectionRegistryCreator factory :
ServiceLoader.load(ConnectionRegistryCreator.class)) {
+ builder.put(factory.protocol(), factory);
Review Comment:
How should we handle protocol/scheme collisions? I prefer that we throw an
exception rather than last one off the classpath wins.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryCreator.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.client;
+
+import java.io.IOException;
+import java.net.URI;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.security.User;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * For creating different {@link ConnectionRegistry} implementation.
+ */
[email protected]
+public interface ConnectionRegistryCreator {
+
+ ConnectionRegistry create(URI uri, Configuration conf, User user) throws
IOException;
+
+ String protocol();
Review Comment:
nit: follow Bean conventions and call this method `getProtocol()`.
And in fact, this should be `getScheme()`, right?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java:
##########
@@ -17,27 +17,75 @@
*/
package org.apache.hadoop.hbase.client;
-import static
org.apache.hadoop.hbase.HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY;
-
+import java.io.IOException;
+import java.net.URI;
+import java.util.ServiceLoader;
+import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.security.User;
import org.apache.hadoop.hbase.util.ReflectionUtils;
import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap;
/**
- * Factory class to get the instance of configured connection registry.
+ * The entry point for creating a {@link ConnectionRegistry}.
*/
@InterfaceAudience.Private
final class ConnectionRegistryFactory {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConnectionRegistryFactory.class);
+
+ private static final ImmutableMap<String, ConnectionRegistryCreator>
CREATORS;
+ static {
+ ImmutableMap.Builder<String, ConnectionRegistryCreator> builder =
ImmutableMap.builder();
+ for (ConnectionRegistryCreator factory :
ServiceLoader.load(ConnectionRegistryCreator.class)) {
Review Comment:
Do we use `ServiceLoader` anywhere else? I think that this is an okay use of
this runtime feature, however it also means that we're exposing ourselves to
user-provided `ConnectionRegistryCreator` implementations. I suspect that this
is not intentional, given the class is marked `IA.Private`. On the other hand,
this could be a powerful point of extension for HBase clients running in
sophisticated environments.
What do you think about making `ConnectionRegistryCreator` `IA.Public` and
supporting this as a point of client extension?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RpcConnectionRegistryCreator.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.client;
+
+import java.io.IOException;
+import java.net.URI;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.security.User;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Connection registry creator implementation for creating {@link
RpcConnectionRegistry}.
+ */
[email protected]
+public class RpcConnectionRegistryCreator implements ConnectionRegistryCreator
{
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RpcConnectionRegistryCreator.class);
+
+ @Override
+ public ConnectionRegistry create(URI uri, Configuration conf, User user)
throws IOException {
+ assert protocol().equals(uri.getScheme());
+ LOG.debug("connect to hbase cluster with rpc bootstrap servers='{}'",
uri.getAuthority());
+ Configuration c = new Configuration(conf);
+ c.set(RpcConnectionRegistry.BOOTSTRAP_NODES, uri.getAuthority());
+ return new RpcConnectionRegistry(c, user);
+ }
+
+ @Override
+ public String protocol() {
+ return "hbase+rpc";
Review Comment:
This part gets messy. I think the convention that has arisen around URI's
with a '+' in their scheme section is used to indicate a protocol+transport.
What does that mean for us? What do we even call our current RPC implementation
of "Hadoop RPC plus protobuf cell back-channel", just "HBase RPC", so
`hbaserpc`, which maybe is short for `hbaserpc+tcp`? Say down the road we
support an hbase rpc over HTTP or over GRPC (which itself supports http/2 and
http/3 as transports, as well as grpc-web) or over UDP, what then? What do we
call our existing Thrift ("hbase+thrift" ?) and REST gateways ("rest+http(s)" ?)
By contrast, the zookeeper client isn't a protocol at all. It's just a
location of an expected service type. So then we can call it just "zookeeper"
or "zk" for short.
I'm not saying we expect to have all these transport mechanisms, but we
should think through what we want this part of our public API to look like and
give precise meaning to the scheme section of the URI.
--
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]
