You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/05/17 14:26:42 UTC
[GitHub] [incubator-ratis] runzhiwang opened a new pull request #102: Fix Failed UT: testRaftServerMetrics
runzhiwang opened a new pull request #102:
URL: https://github.com/apache/incubator-ratis/pull/102
**What's the problem ?**
**What's the reason ?**
One server was elected as leader twice. Every time leader will try to [addNumPendingRequestsByteSize {registry.gauge(REQUEST_BYTE_SIZE, () -> byteSize) } ](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/impl/PendingRequests.java#L91) with a new `RequestLimits`.
But in the second time, it can not re-guage again, because the gauge in metrics was implemented by `getOrAdd` as the following code shows. So it will first check whether `REQUEST_BYTE_SIZE` exist, if exist, it will not `gauge` it again. So the `byteSize` is the old RequestLimits's, and can not update when `addPendingRequest`, so assert failed.
```
public Gauge gauge(String name, final MetricSupplier<Gauge> supplier) {
return getOrAdd(name, new MetricBuilder<Gauge>() {
@Override
public Gauge newMetric() {
return supplier.newMetric();
}
@Override
public boolean isInstance(Metric metric) {
return Gauge.class.isInstance(metric);
}
});
}
```
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #102: RATIS-939. Fix Failed UT: testRaftServerMetrics
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #102:
URL: https://github.com/apache/incubator-ratis/pull/102#discussion_r432906757
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PendingRequests.java
##########
@@ -87,7 +87,10 @@ void release(Message message) {
this.resource = new RequestLimits(elementLimit, byteLimit);
this.raftServerMetrics = raftServerMetrics;
+ raftServerMetrics.removeNumPendingRequestsGauge();
raftServerMetrics.addNumPendingRequestsGauge(resource::getElementCount);
+
Review comment:
@bshashikant I have updated it.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bshashikant commented on pull request #102: RATIS-939. Fix Failed UT: testRaftServerMetrics
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #102:
URL: https://github.com/apache/incubator-ratis/pull/102#issuecomment-637410681
@runzhiwang , can you please update the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] bshashikant merged pull request #102: RATIS-939. Fix Failed UT: testRaftServerMetrics
Posted by GitBox <gi...@apache.org>.
bshashikant merged pull request #102:
URL: https://github.com/apache/incubator-ratis/pull/102
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bshashikant commented on a change in pull request #102: RATIS-939. Fix Failed UT: testRaftServerMetrics
Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #102:
URL: https://github.com/apache/incubator-ratis/pull/102#discussion_r426381181
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PendingRequests.java
##########
@@ -87,7 +87,10 @@ void release(Message message) {
this.resource = new RequestLimits(elementLimit, byteLimit);
this.raftServerMetrics = raftServerMetrics;
+ raftServerMetrics.removeNumPendingRequestsGauge();
raftServerMetrics.addNumPendingRequestsGauge(resource::getElementCount);
+
Review comment:
I think it would be better to remove the metrics when the leader steps down instead of removing and then adding it back ? what do you think?
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bshashikant commented on pull request #102: RATIS-939. Fix Failed UT: testRaftServerMetrics
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #102:
URL: https://github.com/apache/incubator-ratis/pull/102#issuecomment-637999482
Thanks @runzhiwang for the contribution. I have committed this.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on pull request #102: RATIS-939. Fix Failed UT: testRaftServerMetrics
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #102:
URL: https://github.com/apache/incubator-ratis/pull/102#issuecomment-637444715
@bshashikant updated
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on pull request #102: Fix Failed UT: testRaftServerMetrics
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #102:
URL: https://github.com/apache/incubator-ratis/pull/102#issuecomment-629807514
@bshashikant @lokeshj1703 Could you help review this patch ? Thank you very much.
----------------------------------------------------------------
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