phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William Yang (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (PHOENIX-3360) Secondary index configuration is wrong
Date Tue, 14 Feb 2017 08:29:41 GMT

    [ https://issues.apache.org/jira/browse/PHOENIX-3360?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15865236#comment-15865236
] 

William Yang edited comment on PHOENIX-3360 at 2/14/17 8:28 AM:
----------------------------------------------------------------

bq. CompoundConfiguration treats the added configs as immutable, and has an internal mutable
config (see the code). This means that with the original patch, the rest of region server
(including replication) will not be affected.

I've done a simple test, see {{ConfCP.java}}. If we changed the RegionServer level configuration
in a region's coprocessor, then all the other Regions opened afterwards on the same RS will
see the change. It has nothing to do with the implementation of Configuration class or any
other internal classes, but is determined by where a region's Configuration object comes from.


I checked the code in both hbase 1.1.2 and 0.94. See {{RegionCoprocessorHost#getTableCoprocessorAttrsFromSchema()}}
for 1.1 and {{RegionCoprocessorHost#loadTableCoprocessors()}} for 0.94. 

Each region will have its own copy of Configuration, which are all copied from the region
server's configuration object. So it is safe to change the configuration returned by {{CoprocessorEnvironment#getConfiguration()}}
and this change can be seen only within this Region. But we should never change the Configurations
return by {{RegionCoprocessorEnvironment#getRegionServerServices().getConfiguration()}} for
this will change all the other Regions' conf (Only the regions being opened afterwards).

How to use ConfCP.java
 * create 'test1', 'cf'
 * create 'test2', 'cf'
 * make sure that all regions of the above two tables are hosted in the same regionserver
 * add coprocessor ConfCP  for test1, check log, should see the print below:
{code}
YHYH1: [test1]conf hashCode = 2027310658
YHYH2: [test1]put conf (yh.special.key,XXXXXX)
YHYH3: [test1]get conf (yh.special.key,XXXXXX)
{code}
 * add coprocessor ConfCP for test2, check the log again, should see the print below
{code}
YHYH1: [test2]conf hashCode = 2027310658
YHYH3: [test2]get conf (yh.special.key,XXXXXX)
{code}

So we set a unique conf in coprocessor in test1, then test2 saw it.
Note that {{conf}} can be assigned in two ways. Currently 
{code}
conf = ((RegionCoprocessorEnvironment)e).getRegionServerServices().getConfiguration();
{code}
is used, and this is what we do in V1 patch.

Change it to 
{code}
conf = e.getConfiguration();
{code}
then table test2 will not see the change that test1 did.

Above all, we can use the v1 patch with a little modification that we just set the conf returned
by {{CoprocessorEnvironment#getConfiguration()}}.  And for PHOENIX-3271 that UPSART SELECT's
write will still have higher priority. 

WDYT? Ping [~jamestaylor], [~enis], [~rajeshbabu].



was (Author: yhxx511):
bq. CompoundConfiguration treats the added configs as immutable, and has an internal mutable
config (see the code). This means that with the original patch, the rest of region server
(including replication) will not be affected.

I've done a simple test, see {{ConfCP.java}}. If we change the RegionServer level configuration
in a coprocessor, then all the other Regions opened on the same RS will see the change. It
has nothing to do with the implementation of Configuration class or any other internal classes,
but is determined by where a region's Configuration object comes from. 

I checked the code in both hbase 1.1.2 and 0.94. See {{RegionCoprocessorHost#getTableCoprocessorAttrsFromSchema()}}
for 1.1 and {{RegionCoprocessorHost#loadTableCoprocessors()}} for 0.94. 

Each region will have its own copy of Configuration, which are all copied from the region
server's configuration object. So it is safe to change the configuration returned by {{CoprocessorEnvironment#getConfiguration()}}
and this change can be seen only within this Region. But we should never change the Configurations
return by {{RegionCoprocessorEnvironment#getRegionServerServices().getConfiguration()}} for
this will change all the other Regions' conf.

How to use ConfCP.java
 * create 'test1', 'cf'
 * create 'test2', 'cf'
 * make sure that all regions of the above two tables are hosted in the same regionserver
 * add coprocessor ConfCP  for test1, check log, should see the print below:
{code}
YHYH1: [test1]conf hashCode = 2027310658
YHYH2: [test1]put conf (yh.special.key,XXXXXX)
YHYH3: [test1]get conf (yh.special.key,XXXXXX)
{code}
 * add coprocessor ConfCP for test2, check the log again, should see the print below
{code}
YHYH1: [test2]conf hashCode = 2027310658
YHYH3: [test2]get conf (yh.special.key,XXXXXX)
{code}

Note that {{conf}} can be assigned by two values. for 
{code}
conf = ((RegionCoprocessorEnvironment)e).getRegionServerServices().getConfiguration();
{code}
is used now, and this is what we do in V1 patch.

Change it to 
{code}
conf = e.getConfiguration();
{code}
then table test2 will not see the change that test1 did.

Above all, we can use the v1 patch with a little modification that we just set the conf returned
by {{CoprocessorEnvironment#getConfiguration()}}.  And for PHOENIX-3271 that UPSART SELECT's
write will still have higher priority. 

WDYT? Ping [~jamestaylor], [~enis], [~rajeshbabu].


> Secondary index configuration is wrong
> --------------------------------------
>
>                 Key: PHOENIX-3360
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-3360
>             Project: Phoenix
>          Issue Type: Bug
>            Reporter: Enis Soztutar
>            Assignee: Rajeshbabu Chintaguntla
>            Priority: Critical
>             Fix For: 4.10.0
>
>         Attachments: ConfCP.java, PHOENIX-3360.patch, PHOENIX-3360-v2.PATCH, PHOENIX-3360-v3.PATCH,
PHOENIX-3360-v4.PATCH
>
>
> IndexRpcScheduler allocates some handler threads and uses a higher priority for RPCs.
The corresponding IndexRpcController is not used by default as it is, but used through ServerRpcControllerFactory
that we configure from Ambari by default which sets the priority of the outgoing RPCs to either
metadata priority, or the index priority.
> However, after reading code of IndexRpcController / ServerRpcController it seems that
the IndexRPCController DOES NOT look at whether the outgoing RPC is for an Index table or
not. It just sets ALL rpc priorities to be the index priority. The intention seems to be the
case that ONLY on servers, we configure ServerRpcControllerFactory, and with clients we NEVER
configure ServerRpcControllerFactory, but instead use ClientRpcControllerFactory. We configure
ServerRpcControllerFactory from Ambari, which in affect makes it so that ALL rpcs from Phoenix
are only handled by the index handlers by default. It means all deadlock cases are still there.

> The documentation in https://phoenix.apache.org/secondary_indexing.html is also wrong
in this sense. It does not talk about server side / client side. Plus this way of configuring
different values is not how HBase configuration is deployed. We cannot have the configuration
show the ServerRpcControllerFactory even only for server nodes, because the clients running
on those nodes will also see the wrong values. 



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Mime
View raw message