You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/06/09 16:33:49 UTC
[GitHub] [cassandra] adelapena commented on a change in pull request #606: Cassandra 15752 trunk
adelapena commented on a change in pull request #606:
URL: https://github.com/apache/cassandra/pull/606#discussion_r437334389
##########
File path: src/java/org/apache/cassandra/service/StorageProxy.java
##########
@@ -2100,22 +2112,33 @@ public RowIterator computeNext()
}
}
- private void updateConcurrencyFactor()
+ @VisibleForTesting
+ public void handleBatchCompleted()
Review comment:
+1 to place update of `liveReturned` here. AFIK this method doesn't do anything else than updating the concurrency factor, and we don't have any alternative implementations, so I think I'd prefer the old name for this method, `updateConcurrencyFactor`. As for the visibility change, I don't see where is it used in testing.
##########
File path: src/java/org/apache/cassandra/locator/ReplicaPlan.java
##########
@@ -106,18 +106,25 @@ ForTokenRead withContact(EndpointsForToken newContact)
public static class ForRangeRead extends ForRead<EndpointsForRange>
{
final AbstractBounds<PartitionPosition> range;
+ final int rangeCount;
- public ForRangeRead(Keyspace keyspace, ConsistencyLevel consistencyLevel, AbstractBounds<PartitionPosition> range, EndpointsForRange candidates, EndpointsForRange contact)
+ public ForRangeRead(Keyspace keyspace, ConsistencyLevel consistencyLevel, AbstractBounds<PartitionPosition> range, EndpointsForRange candidates, EndpointsForRange contact, int rangeCount)
Review comment:
Nit: We could break this line
```suggestion
public ForRangeRead(Keyspace keyspace,
ConsistencyLevel consistencyLevel,
AbstractBounds<PartitionPosition> range,
EndpointsForRange candidates,
EndpointsForRange contact,
int rangeCount)
```
##########
File path: src/java/org/apache/cassandra/locator/ReplicaPlan.java
##########
@@ -106,18 +106,25 @@ ForTokenRead withContact(EndpointsForToken newContact)
public static class ForRangeRead extends ForRead<EndpointsForRange>
{
final AbstractBounds<PartitionPosition> range;
+ final int rangeCount;
- public ForRangeRead(Keyspace keyspace, ConsistencyLevel consistencyLevel, AbstractBounds<PartitionPosition> range, EndpointsForRange candidates, EndpointsForRange contact)
+ public ForRangeRead(Keyspace keyspace, ConsistencyLevel consistencyLevel, AbstractBounds<PartitionPosition> range, EndpointsForRange candidates, EndpointsForRange contact, int rangeCount)
{
super(keyspace, consistencyLevel, candidates, contact);
this.range = range;
+ this.rangeCount = rangeCount;
}
public AbstractBounds<PartitionPosition> range() { return range; }
+ /**
+ * @return number of vnode ranges
Review comment:
Perhaps we could extend this to `number of vnode ranges intersected by the range`, or rename the method to `vnodesCount`/`subrangeCount`, or something like that. I think that having a singular `range` and a range count at the same time might be a bit confusing to unaware readers.
##########
File path: test/unit/org/apache/cassandra/service/reads/repair/AbstractReadRepairTest.java
##########
@@ -294,7 +294,7 @@ public void setUp()
static ReplicaPlan.ForRangeRead replicaPlan(Keyspace keyspace, ConsistencyLevel consistencyLevel, EndpointsForRange replicas, EndpointsForRange targets)
{
return new ReplicaPlan.ForRangeRead(keyspace, consistencyLevel,
- ReplicaUtils.FULL_BOUNDS, replicas, targets);
+ ReplicaUtils.FULL_BOUNDS, replicas, targets, 1);
Review comment:
Nit: I think we don't need to break this line
##########
File path: test/unit/org/apache/cassandra/db/PartitionRangeReadTest.java
##########
@@ -188,5 +201,94 @@ public void testRangeSliceInclusionExclusion() throws Throwable
assertTrue(partitions.get(0).iterator().next().getCell(cDef).value().equals(ByteBufferUtil.bytes("2")));
assertTrue(partitions.get(partitions.size() - 1).iterator().next().getCell(cDef).value().equals(ByteBufferUtil.bytes("6")));
}
+
+ @Test
+ public void testComputeConcurrencyFactor()
+ {
+ int maxConccurrentRangeRequest = 32;
+
+ // no live row returned, fetch all remaining ranges but hit the max instead
+ int cf = StorageProxy.RangeCommandIterator.computeConcurrencyFactor(100, 30, maxConccurrentRangeRequest, 500, 0);
+ assertEquals(maxConccurrentRangeRequest, cf); // because 100 - 30 = 70 > maxConccurrentRangeRequest
Review comment:
I really like the comments here 👍
##########
File path: src/java/org/apache/cassandra/service/StorageProxy.java
##########
@@ -2041,16 +2052,18 @@ public void close()
private DataLimits.Counter counter;
private PartitionIterator sentQueryIterator;
+ private int maxConcurrencyFactor;
private int concurrencyFactor;
// The two following "metric" are maintained to improve the concurrencyFactor
// when it was not good enough initially.
private int liveReturned;
private int rangesQueried;
- public RangeCommandIterator(RangeIterator ranges, PartitionRangeReadCommand command, int concurrencyFactor, Keyspace keyspace, ConsistencyLevel consistency, long queryStartNanoTime)
+ public RangeCommandIterator(RangeIterator ranges, PartitionRangeReadCommand command, int concurrencyFactor, int maxConcurrencyFactor, Keyspace keyspace, ConsistencyLevel consistency, long queryStartNanoTime)
Review comment:
Nit: we could break this line
```suggestion
public RangeCommandIterator(RangeIterator ranges,
PartitionRangeReadCommand command,
int concurrencyFactor,
int maxConcurrencyFactor,
Keyspace keyspace,
ConsistencyLevel consistency,
long queryStartNanoTime)
```
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org