[jira] [Comment Edited] (HBASE-18359) CoprocessorHConnection#getConnectionForEnvironment should read config from CoprocessorEnvironment
[
https://issues.apache.org/jira/browse/HBASE-18359?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16258239#comment-16258239
]
Samarth Jain edited comment on HBASE-18359 at 11/18/17 10:34 PM:
-
We need our connections to have a different configuration than the one in
hbase-site.xml. The current custom configs mostly are related to remote RPCs,
so probably in case of short-circuit we can use the same connection as the RS
one. In future, it may change. So to avoid any such hard to catch regressions,
I think the best approach would be to just let the clients manage the life
cycle of the connection. Explicit documentation on the lines of "You know what
you are doing. And that creating too many connections will make you run out of
ZK resources, etc" would help. In Phoenix we guard against that by caching the
connection.
was (Author: samarthjain):
We need our connections to have a different configuration that the one in
hbase-site.xml. The current custom configs mostly are related to remote RPCs,
so probably in case of short-circuit we can use the same connection as the RS
one. In future, it may change. So to avoid the any such hard to catch
regression, I think the best approach would be to just let the clients manage
the life cycle of the connection. Explicit documentation on the lines of "You
know what you are doing. And that creating too many connections will make you
run out of ZK resources, etc" would help. In Phoenix we guard against that by
caching the connection.
> CoprocessorHConnection#getConnectionForEnvironment should read config from
> CoprocessorEnvironment
> -
>
> Key: HBASE-18359
> URL: https://issues.apache.org/jira/browse/HBASE-18359
> Project: HBase
> Issue Type: Bug
>Reporter: Samarth Jain
> Fix For: 2.0.0
>
>
> It seems like the method getConnectionForEnvironment isn't doing the right
> thing when it is creating a CoprocessorHConnection by reading the config from
> HRegionServer and not from the env passed in.
> If coprocessors want to use a CoprocessorHConnection with some custom config
> settings, then they have no option but to configure it in the hbase-site.xml
> of the region servers. This isn't ideal as a lot of times these "global"
> level configs can have side effects. See PHOENIX-3974 as an example where
> configuring ServerRpcControllerFactory (a Phoenix implementation of
> RpcControllerFactory) could result in deadlocks. Or PHOENIX-3983 where
> presence of this global config causes our index rebuild code to incorrectly
> use handlers it shouldn't.
> If the CoprocessorHConnection created through getConnectionForEnvironment API
> used the CoprocessorEnvironment config, then it would allow co-processors to
> pass in their own config without needing to configure them in hbase-site.xml.
> The change would be simple. Basically change the below
> {code}
> if (services instanceof HRegionServer) {
> return new CoprocessorHConnection((HRegionServer) services);
> }
> {code}
> to
> {code}
> if (services instanceof HRegionServer) {
> return new CoprocessorHConnection(env.getConfiguration(),
> (HRegionServer) services);
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Comment Edited] (HBASE-18359) CoprocessorHConnection#getConnectionForEnvironment should read config from CoprocessorEnvironment
[
https://issues.apache.org/jira/browse/HBASE-18359?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16257476#comment-16257476
]
Samarth Jain edited comment on HBASE-18359 at 11/17/17 7:50 PM:
bq. On older versions also it is not as the
CoprocessorHConnection#getConnectionForEnvironment was taking a
CoprocessorEnvironment instance not config
The issue is present in older versions. Instead of using the env passed in, the
code was using the environment of HRegionServer which is what the description
also talks about.
{code}
if (env instanceof RegionCoprocessorEnvironment) {
RegionCoprocessorEnvironment e = (RegionCoprocessorEnvironment) env;
RegionServerServices services = e.getRegionServerServices();
if (services instanceof HRegionServer) {
return new CoprocessorHConnection((HRegionServer) services);
}
}
{code}
bq. What we need in HBase is an API
CoprocessorEnvironment#getConnection(Config). This call will always make new
connection (which is short circuit enabled). So caching of this and reuse the
callee has to take care. Is that ok?
I think that should work, [~anoop.hbase]. We already cache the HConnection in
CoprocessorHConnectionTableFactory by doing this:
{code}
private synchronized HConnection getConnection(Configuration conf) throws
IOException {
if (connection == null || connection.isClosed()) {
connection = new CoprocessorHConnection(conf, server);
}
return connection;
}
{code}
It would be good if the API has explicit documentation saying it is the
caller's responsibility to make sure the connection returned by the
getConnection(config) API is appropriately closed.
was (Author: samarthjain):
bq. On older versions also it is not as the
CoprocessorHConnection#getConnectionForEnvironment was taking a
CoprocessorEnvironment instance not config
The issue is present in older versions. Instead of using the env passed in, the
code was using the environment of HRegionServer which is what the description
also talks about.
{code
if (env instanceof RegionCoprocessorEnvironment) {
RegionCoprocessorEnvironment e = (RegionCoprocessorEnvironment) env;
RegionServerServices services = e.getRegionServerServices();
if (services instanceof HRegionServer) {
return new CoprocessorHConnection((HRegionServer) services);
}
}
{code}
bq. What we need in HBase is an API
CoprocessorEnvironment#getConnection(Config). This call will always make new
connection (which is short circuit enabled). So caching of this and reuse the
callee has to take care. Is that ok?
I think that should work, [~anoop.hbase]. We already cache the HConnection in
CoprocessorHConnectionTableFactory by doing this:
{code}
private synchronized HConnection getConnection(Configuration conf) throws
IOException {
if (connection == null || connection.isClosed()) {
connection = new CoprocessorHConnection(conf, server);
}
return connection;
}
{code}
It would be good if the API has explicit documentation saying it is the
caller's responsibility to make sure the connection returned by the
getConnection(config) API is appropriately closed.
> CoprocessorHConnection#getConnectionForEnvironment should read config from
> CoprocessorEnvironment
> -
>
> Key: HBASE-18359
> URL: https://issues.apache.org/jira/browse/HBASE-18359
> Project: HBase
> Issue Type: Bug
>Reporter: Samarth Jain
> Fix For: 2.0.0
>
>
> It seems like the method getConnectionForEnvironment isn't doing the right
> thing when it is creating a CoprocessorHConnection by reading the config from
> HRegionServer and not from the env passed in.
> If coprocessors want to use a CoprocessorHConnection with some custom config
> settings, then they have no option but to configure it in the hbase-site.xml
> of the region servers. This isn't ideal as a lot of times these "global"
> level configs can have side effects. See PHOENIX-3974 as an example where
> configuring ServerRpcControllerFactory (a Phoenix implementation of
> RpcControllerFactory) could result in deadlocks. Or PHOENIX-3983 where
> presence of this global config causes our index rebuild code to incorrectly
> use handlers it shouldn't.
> If the CoprocessorHConnection created through getConnectionForEnvironment API
> used the CoprocessorEnvironment config, then it would allow co-processors to
> pass in their own config without needing to configure them in hbase-site.xml.
> The change would be simple. Basically change the below
> {code}
> if (services instanceof HRegionServer) {
> return new CoprocessorHConnection((HRegionServer) services);
> }
>
