[
https://issues.apache.org/jira/browse/PHOENIX-6127?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17236514#comment-17236514
]
ASF GitHub Bot commented on PHOENIX-6127:
-----------------------------------------
ChinmaySKulkarni commented on a change in pull request #944:
URL: https://github.com/apache/phoenix/pull/944#discussion_r528022575
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
##########
@@ -2458,6 +2455,35 @@ private MetaDataResponse
processRemoteRegionMutations(byte[] systemTableName,
return null;
}
+ private MetaDataResponse
processRemoteRegionMutationsOnSystemChildLinkTable(
+ List<Mutation> remoteMutations, MetaDataProtos.MutationCode
mutationCode)
+ throws IOException {
+ if (remoteMutations.isEmpty())
Review comment:
nit: Please use `{` `}` braces even if it is a single-line `if` condition
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ViewUtil.java
##########
@@ -353,14 +396,20 @@ public static void
dropChildViews(RegionCoprocessorEnvironment env, byte[] tenan
Table hTable = null;
try {
hTable = ServerUtil.getHTableForCoprocessorScan(env,
SchemaUtil.getPhysicalTableName(
- sysCatOrSysChildLink, env.getConfiguration()));
+ sysCatOrSysChildLink, env.getConfiguration()));
} catch (Exception e){
logger.error("ServerUtil.getHTableForCoprocessorScan error!", e);
}
// if the SYSTEM.CATALOG or SYSTEM.CHILD_LINK doesn't exist just return
if (hTable==null) {
return;
}
+ dropChildViewsCommon(env, tenantIdBytes, schemaName, tableOrViewName,
hTable);
+ }
+
+ public static void dropChildViewsCommon(RegionCoprocessorEnvironment env,
byte[] tenantIdBytes,
Review comment:
can we make this `private`?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ViewUtil.java
##########
@@ -337,6 +337,49 @@ public boolean filterAllRemaining() {
/**
* Attempt to drop an orphan child view i.e. a child view for which we see
a parent->child entry
* in SYSTEM.CHILD_LINK/SYSTEM.CATALOG (as a child) but for whom the
parent no longer exists.
+ * Its better to use this version, and trying to get the hTable Lazily
instead of checking it
+ * first via HBase admin call
+ * @param env Region Coprocessor environment
+ * @param tenantIdBytes tenantId of the parent
+ * @param schemaName schema of the parent
+ * @param tableOrViewName parent table/view name
+ * @throws IOException thrown if there is an error scanning
SYSTEM.CHILD_LINK or SYSTEM.CATALOG
+ * @throws SQLException thrown if there is an error getting a connection
to the server or
+ * an error retrieving the PTable for a child view
+ */
+ public static void dropChildViews(RegionCoprocessorEnvironment env, byte[]
tenantIdBytes,
+ byte[] schemaName, byte[]
tableOrViewName)
+ throws IOException, SQLException {
+ Configuration conf = env.getConfiguration();
+ Table hTable = null;
+ try {
+ byte[] sysCatOrSysChildLink =
SchemaUtil.getPhysicalTableName(SYSTEM_CATALOG_NAME_BYTES,
Review comment:
The order needs to be reversed here. We should first try to get
`SYSTEM.CHILD_LINK` and if that fails, fallback to using `SYSTEM.CATALOG`. We
also need to maintain the client version logic that
[ViewUtil.getSystemTableForChildLinks()](https://github.com/apache/phoenix/blob/bf8bb519b0c3d0fe96e7e1e33a7672fc0e15b1ad/phoenix-core/src/main/java/org/apache/phoenix/util/ViewUtil.java#L497)
has. Btw, with your changes, do we need that API anymore?
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
##########
@@ -2239,8 +2238,7 @@ public void dropTable(RpcController controller,
DropTableRequest request,
if (pTableType == PTableType.TABLE || pTableType ==
PTableType.VIEW) {
// check to see if the table has any child views
- try (Table hTable = ServerUtil.getHTableForCoprocessorScan(env,
- getSystemTableForChildLinks(clientVersion,
env.getConfiguration()))) {
+ try (Table hTable = ServerUtil.getHTableSystemChildLink(env)) {
Review comment:
nit: Rename to `getHTableSysChildLinkOrSysCat` or something to indicate
that we may get the latter too
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/ViewUtil.java
##########
@@ -337,6 +337,49 @@ public boolean filterAllRemaining() {
/**
* Attempt to drop an orphan child view i.e. a child view for which we see
a parent->child entry
* in SYSTEM.CHILD_LINK/SYSTEM.CATALOG (as a child) but for whom the
parent no longer exists.
+ * Its better to use this version, and trying to get the hTable Lazily
instead of checking it
+ * first via HBase admin call
+ * @param env Region Coprocessor environment
+ * @param tenantIdBytes tenantId of the parent
+ * @param schemaName schema of the parent
+ * @param tableOrViewName parent table/view name
+ * @throws IOException thrown if there is an error scanning
SYSTEM.CHILD_LINK or SYSTEM.CATALOG
+ * @throws SQLException thrown if there is an error getting a connection
to the server or
+ * an error retrieving the PTable for a child view
+ */
+ public static void dropChildViews(RegionCoprocessorEnvironment env, byte[]
tenantIdBytes,
+ byte[] schemaName, byte[]
tableOrViewName)
+ throws IOException, SQLException {
+ Configuration conf = env.getConfiguration();
+ Table hTable = null;
+ try {
+ byte[] sysCatOrSysChildLink =
SchemaUtil.getPhysicalTableName(SYSTEM_CATALOG_NAME_BYTES,
+ conf).getName();
+ hTable = ServerUtil.getHTableForCoprocessorScan(env,
SchemaUtil.getPhysicalTableName(
+ sysCatOrSysChildLink, env.getConfiguration()));
+ } catch (Exception e){
+ try {
+ byte[] sysCatOrSysChildLink = SchemaUtil.getPhysicalTableName(
+ SYSTEM_CHILD_LINK_NAME_BYTES, conf).getName();
+ hTable = ServerUtil.getHTableForCoprocessorScan(env,
+ SchemaUtil.getPhysicalTableName(sysCatOrSysChildLink,
+ env.getConfiguration()));
+ } catch (Exception e1){
+ logger.error("ServerUtil.getHTableForCoprocessorScan error!",
e);
+ }
+ }
+ // if the SYSTEM.CATALOG or SYSTEM.CHILD_LINK doesn't exist just return
+ if (hTable==null) {
Review comment:
We should log an error here.
----------------------------------------------------------------
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:
[email protected]
> Prevent unnecessary HBase admin API calls in
> ViewUtil.getSystemTableForChildLinks() and act lazily instead
> ----------------------------------------------------------------------------------------------------------
>
> Key: PHOENIX-6127
> URL: https://issues.apache.org/jira/browse/PHOENIX-6127
> Project: Phoenix
> Issue Type: Improvement
> Affects Versions: 5.0.0, 4.15.0
> Reporter: Chinmay Kulkarni
> Assignee: Richard Antal
> Priority: Major
> Labels: phoenix-hardening, quality-improvement
> Fix For: 5.1.0, 4.16.0
>
> Attachments: PHOENIX-6127.master.v1.patch
>
>
> In order to handle the case of older clients connecting to a 4.16 cluster
> that has old metadata (no SYSTEM.CHILD_LINK table yet), we call
> ViewUtil.getSystemTableForChildLinks() to figure out whether to use
> SYSTEM.CHILD_LINK or SYSTEM.CATALOG to look up parent->child linking rows.
> Here we do HBase table existence checks using HBase admin APIs (see
> [this|https://github.com/apache/phoenix/blob/e3c7b4bdce2524eb4fd1e7eb0ccd3454fcca81ce/phoenix-core/src/main/java/org/apache/phoenix/util/ViewUtil.java#L265-L269])
> which can be avoided. In almost all cases once we've called this API, we
> later go on and retrieve the Table object anyhow so we can instead try to
> always get the SYSTEM.CHILD_LINK table and if that fails, try to get
> SYSTEM.CATALOG. This will avoid additional admin API calls.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
