JAkutenshi commented on code in PR #6880:
URL: https://github.com/apache/ignite-3/pull/6880#discussion_r2494406502
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/metrics/PlacementDriverMetricSource.java:
##########
@@ -33,122 +33,71 @@ public class PlacementDriverMetricSource extends
AbstractMetricSource<Holder> {
/** Source name. */
public static final String SOURCE_NAME = "placement-driver";
- private final IntSupplier activeLeaseSupplier;
- private final IntSupplier leaseWithoutCandidatesSupplier;
- private final IntSupplier currentStableAssignmentSizeSupplier;
- private final IntSupplier currentPendingAssignmentSizeSupplier;
+ private final LeaseTracker leaseTracker;
+ private final AssignmentsTracker assignmentsTracker;
/**
* Constructor.
*
- * @param activeLeaseSupplier Supplier for active leases count.
- * @param leaseWithoutCandidatesSupplier Supplier for leases without
candidates count.
- * @param currentStableAssignmentSizeSupplier Supplier for the stable
assignments count.
- * @param currentPendingAssignmentSizeSupplier Supplier for the pending
assignments count.
+ * @param leaseTracker Lease tracker.
+ * @param assignmentsTracker Assignments tracker.
*/
public PlacementDriverMetricSource(
- IntSupplier activeLeaseSupplier,
- IntSupplier leaseWithoutCandidatesSupplier,
- IntSupplier currentStableAssignmentSizeSupplier,
- IntSupplier currentPendingAssignmentSizeSupplier
+ LeaseTracker leaseTracker,
+ AssignmentsTracker assignmentsTracker
) {
super(SOURCE_NAME, "Placement driver metrics.");
- this.activeLeaseSupplier = activeLeaseSupplier;
- this.leaseWithoutCandidatesSupplier = leaseWithoutCandidatesSupplier;
- this.currentStableAssignmentSizeSupplier =
currentStableAssignmentSizeSupplier;
- this.currentPendingAssignmentSizeSupplier =
currentPendingAssignmentSizeSupplier;
+ this.leaseTracker = leaseTracker;
+ this.assignmentsTracker = assignmentsTracker;
}
@Override
protected Holder createHolder() {
return new Holder();
}
- /**
- * Is called on lease creation.
- */
- public void onLeaseCreate() {
- Holder holder = holder();
- if (holder != null) {
- holder.leasesCreated.increment();
- }
- }
-
- /**
- * Is called on lease prolongation.
- */
- public void onLeaseProlong() {
- Holder holder = holder();
- if (holder != null) {
- holder.leasesProlonged.increment();
- }
- }
-
- /**
- * Is called on lease publishing.
- */
- public void onLeasePublish() {
- Holder holder = holder();
- if (holder != null) {
- holder.leasesPublished.increment();
- }
- }
-
/** Holder. */
protected class Holder implements AbstractMetricSource.Holder<Holder> {
- private final LongAdderMetric leasesCreated = new LongAdderMetric(
- "LeasesCreated",
- "Total number of created leases."
- );
-
- private final LongAdderMetric leasesPublished = new LongAdderMetric(
- "LeasesPublished",
- "Total number of published leases."
- );
-
- private final LongAdderMetric leasesProlonged = new LongAdderMetric(
- "LeasesProlonged",
- "Total number of prolonged leases."
- );
-
- private final IntMetric activeLeaseCount = new IntGauge(
- "ActiveLeasesCount",
- "Number of currently active leases.",
- activeLeaseSupplier
+ private final IntGauge acceptedLeases = new IntGauge(
+ "AcceptedLeases",
+ "Number of accepted leases.",
+ () -> numberOfLeases(true)
);
- private final IntMetric leaseWithoutCandidates = new IntGauge(
- "LeasesWithoutCandidates",
- "Number of leases without candidates currently existing.",
- leaseWithoutCandidatesSupplier
+ private final IntGauge leaseNegotiations = new IntGauge(
+ "LeaseNegotiations",
+ "Number of leases under negotiation.",
+ () -> numberOfLeases(false)
);
- private final IntMetric currentStableAssignmentSize = new IntGauge(
- "CurrentStableAssignmentsSize",
- "Current size of stable assignments over all partitions.",
- currentStableAssignmentSizeSupplier
- );
-
- private final IntMetric currentPendingAssignmentSize = new IntGauge(
- "CurrentPendingAssignmentsSize",
- "Current size of pending assignments over all partitions.",
- currentPendingAssignmentSizeSupplier
+ private final IntGauge replicationGroups = new IntGauge(
+ "ReplicationGroups",
+ "Current number of replication groups.",
+ () -> assignmentsTracker.stableAssignments().size()
);
private final List<Metric> metrics = List.of(
- leasesCreated,
- leasesPublished,
- leasesProlonged,
- leaseWithoutCandidates,
- activeLeaseCount,
- currentStableAssignmentSize,
- currentPendingAssignmentSize
+ acceptedLeases,
+ leaseNegotiations,
+ replicationGroups
);
@Override
public Iterable<Metric> metrics() {
return metrics;
}
+
+ private int numberOfLeases(boolean accepted) {
Review Comment:
I believe that methods with more verbose name is better than boolean flag,
so I optionally suggest this patch:
```
Index:
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/metrics/PlacementDriverMetricSource.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git
a/modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/metrics/PlacementDriverMetricSource.java
b/modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/metrics/PlacementDriverMetricSource.java
---
a/modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/metrics/PlacementDriverMetricSource.java
(revision 757bd96af184ce201d91c8b026e844bd2ff9a0e9)
+++
b/modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/metrics/PlacementDriverMetricSource.java
(date 1762347955801)
@@ -62,13 +62,13 @@
private final IntGauge acceptedLeases = new IntGauge(
"AcceptedLeases",
"Number of accepted leases.",
- () -> numberOfLeases(true)
+ () -> numberOfAcceptedLeases()
);
private final IntGauge leaseNegotiations = new IntGauge(
"LeaseNegotiations",
"Number of leases under negotiation.",
- () -> numberOfLeases(false)
+ () -> numberOfLeasesUnderNegotiation()
);
private final IntGauge replicationGroups = new IntGauge(
@@ -88,7 +88,17 @@
return metrics;
}
- private int numberOfLeases(boolean accepted) {
+
+ private int numberOfAcceptedLeases() {
+ return numberOfLeasesInternal(true);
+ }
+
+
+ private int numberOfLeasesUnderNegotiation() {
+ return numberOfLeasesInternal(false);
+ }
+
+ private int numberOfLeasesInternal(boolean accepted) {
int count = 0;
for (Lease lease :
leaseTracker.leasesCurrent().leaseByGroupId().values()) {
```
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/AssignmentsTracker.java:
##########
@@ -307,7 +307,7 @@ private CompletableFuture<TokenizedAssignments>
nonEmptyAssignmentFuture(Replica
*
* @return Map replication group id to its stable assignments.
*/
- Map<ReplicationGroupId, TokenizedAssignments> stableAssignments() {
+ public Map<ReplicationGroupId, TokenizedAssignments> stableAssignments() {
Review Comment:
DO we want to make pending assignments getter as public by the way?
##########
modules/placement-driver/src/test/java/org/apache/ignite/internal/placementdriver/metrics/PlacementDriverMetricSourceTest.java:
##########
@@ -23,40 +23,44 @@
import static org.hamcrest.MatcherAssert.assertThat;
import java.util.Set;
-import java.util.function.IntSupplier;
import java.util.stream.StreamSupport;
import org.apache.ignite.internal.metrics.Metric;
import org.apache.ignite.internal.metrics.MetricSet;
+import org.apache.ignite.internal.placementdriver.AssignmentsTracker;
+import org.apache.ignite.internal.placementdriver.leases.LeaseTracker;
import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
/**
* Tests metric source name and metric names.
*/
+@ExtendWith(MockitoExtension.class)
public class PlacementDriverMetricSourceTest extends BaseIgniteAbstractTest {
+ @Mock
+ private LeaseTracker leaseTracker;
+ @Mock
+ private AssignmentsTracker assignmentsTracker;
+
@Test
void testMetricSourceName() {
assertThat(PlacementDriverMetricSource.SOURCE_NAME,
is("placement-driver"));
}
@Test
void testMetricNames() {
Review Comment:
Should (could?) we test them somehow as integration tests?
##########
modules/placement-driver/src/test/java/org/apache/ignite/internal/placementdriver/metrics/PlacementDriverMetricSourceTest.java:
##########
@@ -23,40 +23,44 @@
import static org.hamcrest.MatcherAssert.assertThat;
import java.util.Set;
-import java.util.function.IntSupplier;
import java.util.stream.StreamSupport;
import org.apache.ignite.internal.metrics.Metric;
import org.apache.ignite.internal.metrics.MetricSet;
+import org.apache.ignite.internal.placementdriver.AssignmentsTracker;
+import org.apache.ignite.internal.placementdriver.leases.LeaseTracker;
import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
/**
* Tests metric source name and metric names.
*/
+@ExtendWith(MockitoExtension.class)
public class PlacementDriverMetricSourceTest extends BaseIgniteAbstractTest {
+ @Mock
+ private LeaseTracker leaseTracker;
+ @Mock
+ private AssignmentsTracker assignmentsTracker;
+
@Test
void testMetricSourceName() {
assertThat(PlacementDriverMetricSource.SOURCE_NAME,
is("placement-driver"));
Review Comment:
Do we break any compatibility with such metrics change as this PR?
--
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]
