Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6523: Support for HRpc connection protocol through JDBC URL - #1280
PHOENIX-6523: Support for HRpc connection protocol through JDBC URL#1280rxmie24 wants to merge 20 commits into
Conversation
stoty
commented
Aug 9, 2021
💔 -1 overall
This message was automatically generated. |
| import org.apache.phoenix.thirdparty.com.google.common.collect.Maps; | ||
| import javax.annotation.concurrent.Immutable; | ||
| import java.io.IOException; | ||
| import java.sql.*; |
There was a problem hiding this comment.
We do not use import * in phoenix code guidelines, you can use our eclipse template or similar.
| this(zookeeperQuorum, port, rootNode, principal, keytab, null); | ||
| } | ||
| public ConnectionInfo(String zookeeperQuorum, Integer port, String rootNode, String principal, String keytab, String bootstrap) { |
There was a problem hiding this comment.
Can we consider a parsed Enum or was there a reason you used a String for bootstrap?
There was a problem hiding this comment.
Been writing a lot of Python lately. whats an enum?
| public static final String HBASE_CLIENT_KEYTAB = "hbase.myclient.keytab"; | ||
| public static final String HBASE_CLIENT_PRINCIPAL = "hbase.myclient.principal"; | ||
| public static final String HBASE_MASTERS = "hbase.masters"; |
There was a problem hiding this comment.
Is this defined already in a publicly facing hbase module? If so can we directly include it via hbase compat or similar?
| import java.sql.DriverManager; | ||
| import java.sql.SQLException; | ||
| import static org.junit.Assert.*; |
stoty
commented
Aug 10, 2021
💔 -1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
stoty
commented
Aug 12, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Aug 12, 2021
💔 -1 overall
This message was automatically generated. |
joshelser
commented
Aug 14, 2021
What is "hrpc" supposed to stand for in this case? I would think "masters" or "registry" might be more clear? I think the problem here is that @bharathv built the "default" implementation to be pluggable. While this is nice, do we have any expectations to support any possible mechanism? It would be helpful (for me, a reviewer) to understand what is in scope and what's out of scope. I'm assuming you don't want to try to support any registry, but it might be a good idea to keep that in mind so as to not code ourselves into a corner. |
joshelser
left a comment
There was a problem hiding this comment.
The enum definitely cleans things up a little, but I think going a little farther to add interface and implementations for ZK and MasterRegistry (and how to parse the rest of the URL, then connect to HBase using that implementation) would go a very long way to clean up PhoenixEmbeddedDriver.
| // ex: +hrpc:hostname1.... | ||
| if (url.startsWith(String.valueOf(PhoenixRuntime.JDBC_PROTOCOL_CONNECTOR_PREFIX))) { | ||
| final String firstToken = url.split(String.valueOf(PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR))[0]; |
There was a problem hiding this comment.
Suggest not using String.split(String) as this ends up being a regex operation.
Please use int offset = url.indexOf(PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR) and url.subString(0, offset) to extract the first piece (or use a tokenizer like elsewhere in this file).
| if (this.isHRPCBootstrap() && getQuorum() != null) { | ||
| final String[] masters = getQuorum().split(","); |
There was a problem hiding this comment.
While the enum suggestion is cleaner, I think pushing this "bootstrapping" into separate classes for each implementation is a pretty standard object-oriented pattern to follow. (e.g. make an interface for PhoenixRuntimeBootstrap with two implementations: one for ZK and one for MasterRegistry).
PhoenixEmbeddedDriver.java is a big mess already and I think pushing even more logic into it will make it worse.
There was a problem hiding this comment.
What is "hrpc" supposed to stand for in this case? I would think "masters" or "registry" might be more clear?
@joshelser I think its short for "hbase rpc". You are probably aware of this discussion [1] that suggests moving the registry hosting out of masters and instead do it in region servers as a part of split meta work. The intent is to not keep masters inline with the RW path. So doing "masters" may not be a good idea in the long term. The new implementation calls it "RpcConnectionRegistry" / "RegionServerRpcConnectionRegistry" depending on the direction we take. But I agree with your point that "hrpc" is a bit opaque.
How about "phoenix+zkregistry" or "phoenix+hbaserpcregistry" ?
| if (this.isHRPCBootstrap() && getQuorum() != null) { | ||
| final String[] masters = getQuorum().split(","); |
rxmie24
commented
Aug 19, 2021
The scope right now is to support HBase registry if specified or else always default to ZK. I think the most important thing here is to ensure there's no regression to existing connections. This HRpc thing should be treated as an opt-in, else we should preserve the same behaviour as before.
I think using some OOP to clean the classes up as you suggested is a good foundation to set for other registries down the road |
stoty
commented
Aug 19, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Aug 19, 2021
💔 -1 overall
This message was automatically generated. |
rxmie24
commented
Aug 26, 2021
@stoty the links to checkstyle/whitespaces are missing Also @joshelser I split and moved the parsing logic into classes |
stoty
commented
Aug 26, 2021
@ramatronics the build artifacts are cleaned up aggressively in a fews days, as the jenkins hosts have little storage. |
rxmie24
commented
Aug 26, 2021
Jenkins test this please |
| import javax.annotation.concurrent.Immutable; | ||
| import org.apache.phoenix.jdbc.bootstrapz.HBaseRegistryBootstrap; |
There was a problem hiding this comment.
can we have just org.apache.phoenix.jdbc.bootstrap. ?
We generally don't use plurals in package names
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @@ -0,0 +1,44 @@ | |||
| package org.apache.phoenix.jdbc.bootstrapz; | |||
There was a problem hiding this comment.
I think that moving parameter parsing to the bootstrap module, and having an API like:
BootstrapRegistry bs = BootstrapFactory.fromURL(String URL);
Map connpProps = bs.generateConnectionProps();
bs.toString()
bs.getZookeperQuorum() (which fails for HRpc)
bs.equals(), bs.hashMap()
would be a better encapsulation, and simplify code.
WDYT @joshelser (as this was originally your suggestion) ?
There was a problem hiding this comment.
It gets a little tricky because the URL is mutated accordingly to tokenize later if there's a +; and we'd be moving that logic into the fromURL method. Also we'd have to move the MalformedUrlException class into its own package to avoid a circular dependency.
There was a problem hiding this comment.
Moving MalformedUrlException to a seprate file is not a problem.
Also the I think that the URL without the boostrap info could be returned as
BootstrapRegistry bs = BootstrapFactory.fromURL(String URL);
Map connpProps = bs.generateConnectionProps();
url = bs.getRemainingURL();
There was a problem hiding this comment.
@stoty check out the updates. I don't think it makes sense to persist the URL in the bootstrap. There's no reason for the BootstrapRegistry object to keep the context of the URL... so I ended up creating a factory method that parses the URL, and spits out a BootstrapRegistry along with the remaining URL in a tuple format.
Let me know if that looks good I'll get started on the integration test with the mini cluster.
There was a problem hiding this comment.
I don't want to hold up the patch, but the current solution feels like the worst of both worlds, as it adds a lots of complexity without removing any.
Classic OO principles say that you should encapsulate your code and data in a class.
The current stateless solution is not worth it. It relies on the fact the state for the ZK and HRpp connections are named similarly, and can be stored in the same variables, but is a bad abstraction.
IMO either just fold back the code into the main class, and save the hassle of using registries, or use stateful registry classes with proper data encapsulation as I suggested above.
@joshelser@yanxinyi what's your take on this ?
There was a problem hiding this comment.
The current stateless solution is not worth it.
you should encapsulate your code and data in a class.
I guess I'm failing to see the opportunity here for encapsulating the code/data. My original intent was to abstract the interface for generateConnectionProps and the logic each registry has for it. Doesn't necessarily mean we have to feed it some data to abstract as well. Sure maybe down the road, it may make sense to move more data from PhoenixEmbeddedDriver into these registry classes as per need... but I don't think any of these classes require context from the URL after being created for the current use case.
relies on the fact the state for the ZK and HRpp connections are named similarly
How? Was hoping to rely on the fact that unique ZK and HRpc identifiers can be parsed into a discrete set of enums. They essentially provide translation of the string value from the conn string to an enum which we then parse into the actual class of the registry. We can eliminate the enum and just add straight string comparisons, but regardless we need to check the protocol string specified in the connection URL.
| import static org.apache.phoenix.thirdparty.com.google.common.base.Preconditions.checkNotNull; | ||
| import static org.apache.phoenix.thirdparty.com.google.common.base.Preconditions.checkArgument; | ||
| import static org.apache.phoenix.thirdparty.com.google.common.collect.Sets.newHashSet; |
stoty
commented
Aug 31, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Sep 10, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Sep 13, 2021
Looking at the code, I'd expect PhoenixEmbeddedDriver.ConnectionInfo.normalize() to fail for Hrpc connections. |
| @@ -0,0 +1,32 @@ | |||
| package org.apache.phoenix.jdbc.bootstrap; | |||
| @@ -0,0 +1,13 @@ | |||
| package org.apache.phoenix.jdbc.bootstrap; | |||
There was a problem hiding this comment.
same as above and rest of new files, plz add apache license header
| "phoenix.query.server.orderBy.spooling.enabled"; | ||
| public static final String HBASE_CLIENT_KEYTAB = "hbase.myclient.keytab"; | ||
| public static final String HBASE_CLIENT_PRINCIPAL = "hbase.myclient.principal"; | ||
| import org.apache.phoenix.thirdparty.org.apache.commons.cli.ParseException; | ||
| import org.apache.commons.lang3.StringEscapeUtils; | ||
| import org.apache.hadoop.hbase.Cell; | ||
| import org.apache.hadoop.hbase.HBaseConfiguration; |
stoty
commented
Sep 20, 2021
💔 -1 overall
This message was automatically generated. |
| import javax.annotation.concurrent.Immutable; | ||
| import org.apache.phoenix.jdbc.bootstrap.*; |
rxmie24
commented
Nov 3, 2021
I moved Registry context to a separate package. I think it covers @joshelser's ask of starting to clean up the Created a class called I think an integration test would really help here so I'm going to get started on that. |
stoty
commented
Jan 2, 2024
A havily modified version of this already been merged. |
HBase now supports a zookeeper-less connection strategy using a Master Registry implementation. For this to work through Phoenix, we need to set a list host:ports of the HMaster quorum
To support opting into this from a Phoenix connection URL, we can introduce a "connector type" as follows:
Above are examples of opting into hrpc/zk registry implementations of HBase.
If no connector is specified, the Phoenix driver will default to a Zookeeper-based connection as it's always been doing prior to this. If an invalid connector type is specified, we throw a
MalformedUrlException, similarly if there's a+char but the connector type is empty.