You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/11/20 23:38:42 UTC
[GitHub] [phoenix] ChinmaySKulkarni commented on a change in pull request #944: PHOENIX-6127 Prevent unnecessary HBase admin API calls in ViewUtil.ge…
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:
users@infra.apache.org