[GitHub] [ignite] alex-plekhanov commented on a diff in pull request #10591: IGNITE-17345 [IEP-35] Metric to track PA enabled request on ThinClient

["alex-plekhanov (via GitHub)" <gi...@apache.org>]

alex-plekhanov commented on code in PR #10591:
URL: https://github.com/apache/ignite/pull/10591#discussion_r1162658617


##########
modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientListenerProcessor.java:
##########
@@ -102,11 +109,21 @@ public class ClientListenerProcessor extends GridProcessorAdapter {
     /** Thin client distributed configuration. */
     private DistributedThinClientConfiguration distrThinCfg;
 
+    /** Awareness hit. */
+    private final LongAdderMetric awarenessHit;
+
+    /** Awareness miss. */
+    private final LongAdderMetric awarenessMiss;
+
     /**
      * @param ctx Kernal context.
      */
     public ClientListenerProcessor(GridKernalContext ctx) {
         super(ctx);
+
+        MetricRegistry mreg = ctx.metric().registry(CLIENT_CONNECTOR_METRICS);
+        awarenessHit = mreg.longAdderMetric(AWARENESS_HIT, "AwarenessHit count.");

Review Comment:
   I'm not sure it's safe to register metrics in constuctor before node start. I think it's better to move metric registration to the `registerClientMetrics` method or to `ClientListenerMetrics` constructor and move `ClientListenerMetrics` from `ClientListenerNioListener` to the `ClientListenerProcessor`



##########
modules/core/src/main/java/org/apache/ignite/internal/processors/platform/client/cache/ClientCacheKeyRequest.java:
##########
@@ -40,10 +42,31 @@ public abstract class ClientCacheKeyRequest extends ClientCacheDataRequest imple
 
     /**
      * Gets the key.
-     *
+     * @param ctx ClientConnectionContext
      * @return Key.
      */
-    public Object key() {
+    public Object key(ClientConnectionContext ctx) {
+        calcAwarenessMetrics(ctx);
+
         return key;
     }
+
+    /** Calculation of awarenes metrics. */
+    protected void calcAwarenessMetrics(ClientConnectionContext ctx) {
+        String cacheName = cacheDescriptor(ctx).cacheName();
+
+        try {
+            Affinity<Object> aff = ctx.kernalContext().affinity().affinityProxy(cacheName);
+
+            if (aff.isPrimary(ctx.kernalContext().discovery().localNode(), key))

Review Comment:
   This code is on the hot path. `affinityProxy` creates new object, then `isAffinity` searches in HashMap twice and also boxes int to Integer. Let's avoid this, by using `F.first(ctx.kernalContext().affinity().mapKeyToPrimaryAndBackups(cacheName, key, null)).isLocal()`, it's more lightweight.
   



##########
modules/core/src/main/java/org/apache/ignite/internal/processors/platform/client/cache/ClientCacheQueryCursor.java:
##########
@@ -79,6 +85,11 @@ void writePage(BinaryRawWriterEx writer) {
             writeEntry(writer, e);
 
             cnt++;
+

Review Comment:
   1. Not sure we need increment metric on each fetch, perhaps processing only start query request is enough.
   2. Queries usually are more rare than other request, perhaps it worth to add dedicated metrics for queries.



##########
modules/core/src/main/java/org/apache/ignite/internal/processors/platform/client/cache/ClientCacheRequest.java:
##########
@@ -169,4 +171,31 @@ public static DynamicCacheDescriptor cacheDescriptor(ClientConnectionContext ctx
     protected int cacheId() {
         return cacheId;
     }
+
+    /**
+     * isAwareness
+     *
+     * @return true if awareness.
+     */
+    protected boolean isAwareness(ClientConnectionContext ctx, String cacheName, Integer part) {
+        boolean awareness = false;
+
+        if (part != null) {
+            try {
+                Affinity<Object> aff = ctx.kernalContext().affinity().affinityProxy(cacheName);
+
+                int[] primaryParts = aff.primaryPartitions(ctx.kernalContext().discovery().localNode());
+
+                if (Arrays.stream(primaryParts).anyMatch(part::equals))
+                    awareness = true;

Review Comment:
   Let's use something like `ctx.kernalContext().affinity().mapPartitionToNode(cacheName, part, null).isLocal()`, it's simpler and faster.



##########
docs/_docs/monitoring-metrics/new-metrics.adoc:
##########
@@ -315,6 +315,8 @@ Register name: `client.connector`
 |SentBytesCount|   long|   Sent bytes count.
 |SslEnabled|   boolean|   Indicates whether SSL is enabled.
 |SslHandshakeDurationHistogram|   histogram|   Histogram of SSL handshake duration in milliseconds (metric is exported only if SSL is enabled).
+|AwarenessHit|   long|   Indicates awareness hit.

Review Comment:
   `Awareness` it's a bad name for these metrics. Awareness of what? I think somethink like `affinityHits` and `affinityMisses` is better.



##########
modules/core/src/main/java/org/apache/ignite/internal/processors/platform/client/cache/ClientCacheClearKeyRequest.java:
##########
@@ -37,7 +37,7 @@ public ClientCacheClearKeyRequest(BinaryRawReaderEx reader) {
     /** {@inheritDoc} */
     @SuppressWarnings("unchecked")
     @Override public ClientResponse process(ClientConnectionContext ctx) {
-        cache(ctx).clear(key());
+        cache(ctx).clear(key(ctx));

Review Comment:
   Is errorprone to increment hits/misses in key getter. Moreover it's not confusing that getter do some extra actions. I think it's better to modify just `ClientCacheKeyRequest.process` method and increment hits/misses here. Also, perhaps we shouldn't count requests inside transactions (but it's discussible), because transaction is binded to some node before first affinity request. 



-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org