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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
