virajjasani commented on code in PR #2321:
URL: https://github.com/apache/phoenix/pull/2321#discussion_r2601800767


##########
phoenix-core-client/src/main/java/org/apache/phoenix/index/IndexMetaDataCacheClient.java:
##########
@@ -120,10 +129,58 @@ public static ServerCache 
setMetaDataOnMutations(PhoenixConnection connection, P
       txState = connection.getMutationState().encodeTransaction();
     }
     boolean hasIndexMetaData = indexMetaDataPtr.getLength() > 0;
+    ReadOnlyProps props = connection.getQueryServices().getProps();
     if (hasIndexMetaData) {
-      if (
+      List<PTable> indexes = table.getIndexes();
+      boolean sendIndexMaintainers = false;
+      if (indexes != null) {
+        for (PTable index : indexes) {
+          if (IndexMaintainer.sendIndexMaintainer(index)) {
+            sendIndexMaintainers = true;
+            break;
+          }
+        }
+      }
+      boolean useServerMetadata = 
props.getBoolean(INDEX_USE_SERVER_METADATA_ATTRIB,
+        QueryServicesOptions.DEFAULT_INDEX_USE_SERVER_METADATA)
+        && props.getBoolean(QueryServices.INDEX_REGION_OBSERVER_ENABLED_ATTRIB,
+          QueryServicesOptions.DEFAULT_INDEX_REGION_OBSERVER_ENABLED);
+      boolean serverSideImmutableIndexes =
+        props.getBoolean(SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
+          DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED);
+      boolean useServerCacheRpc =
         useIndexMetadataCache(connection, mutations, 
indexMetaDataPtr.getLength() + txState.length)
+          && sendIndexMaintainers;
+      long updateCacheFreq = table.getUpdateCacheFrequency();
+      // PHOENIX-7727 Eliminate IndexMetadataCache RPCs by leveraging server 
PTable cache and
+      // retrieve IndexMaintainer objects for each active index from the 
PTable object.
+      // To optimize rpc calls, use it only when all of these conditions are 
met:
+      // 1. Use server metadata feature is enabled (enabled by default).
+      // 2. New index design is used (IndexRegionObserver coproc).
+      // 3. Table is not of type System.
+      // 4. Either table has mutable indexes or server side handling of 
immutable indexes is
+      // enabled.
+      // 5. Table's UPDATE_CACHE_FREQUENCY is not ALWAYS. This ensures 
IndexRegionObserver
+      // does not have to make additional getTable() rpc call with each 
batchMutate() rpc call.
+      // 6. Table's UPDATE_CACHE_FREQUENCY is ALWAYS but addServerCache() rpc 
call is needed
+      // due to the size of mutations. Unless expensive addServerCache() rpc 
call is required,
+      // client can attach index maintainer mutation attribute so that 
IndexRegionObserver
+      // does not have to make additional getTable() rpc call with each 
batchMutate() rpc call
+      // with small mutation size (size < 
phoenix.index.mutableBatchSizeThreshold value).
+      // If above conditions do not match and if the mutation size is greater 
than
+      // "phoenix.index.mutableBatchSizeThreshold" value, however if none of 
the data table
+      // indexes need to be sent to server (only index in state other than 
DISABLE,
+      // CREATE_DISABLE, PENDING_ACTIVE need to be sent to server), do not use 
expensive
+      // addServerCache() rpc call.

Review Comment:
   @tkhurana you have found one of the trickiest part of this work :)
   
   This particular explanation is for a minor behavior change that should be 
applicable to our codebase even without this patch. While `boolean 
hasIndexMetaData = indexMetaDataPtr.getLength() > 0` will ensure the data table 
mutation has some index mutation to send, the condition can still be true 
sometimes, even if no index update is really needed. So far, i was able to 
reproduce it for transform table with namespace enabled use case (though I 
believe the transform table still has a long way to go before it can be 
considered a reliable feature - mostly due to lack of funding).
   
   Does this look good?
   
   > If above conditions (mentioned before L170) are all false and data table 
needs to send index mutation with the data table mutation, we use expensive 
addServerCache() rpc call. However, if above conditions (mentioned before L170) 
are all false and data table mutation does not need to send index mutation 
(because all indexes are in any of DISABLE, CREATE_DISABLE, PENDING_ACTIVE 
states), we can avoid expensive addServerCache() rpc call.



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to