[jira] [Commented] (CASSANDRA-12884) Batch logic can lead to unbalanced use of system.batches
[ https://issues.apache.org/jira/browse/CASSANDRA-12884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16125750#comment-16125750 ] Aleksey Yeschenko commented on CASSANDRA-12884: --- bq. Oh, and not to nitpick, but any reason to prefer {{otherRack.sublist(2, otherRack.size()).clear(); return otherRack();}} to {{return otherRack.sublist(0,2);}}? It's very slightly cheaper as it won't in practice create an extra object. But here it doesn't really matter, and the latter, I agree, reads better. Swapped. Committed to 3.0 as [c2b635ac240ae8d9375fd96791a5aba903a94498|https://github.com/apache/cassandra/commit/c2b635ac240ae8d9375fd96791a5aba903a94498] and merged into 3.11 and trunk. > Batch logic can lead to unbalanced use of system.batches > > > Key: CASSANDRA-12884 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12884 > Project: Cassandra > Issue Type: Bug > Components: Core >Reporter: Adam Hattrell >Assignee: Daniel Cranford > Fix For: 3.0.15, 3.11.1 > > Attachments: 0001-CASSANDRA-12884.patch > > > It looks as though there are some odd edge cases in how we distribute the > copies in system.batches. > The main issue is in the filter method for > org.apache.cassandra.batchlog.BatchlogManager > {code:java} > if (validated.size() - validated.get(localRack).size() >= 2) > { > // we have enough endpoints in other racks > validated.removeAll(localRack); > } > if (validated.keySet().size() == 1) > { >// we have only 1 `other` rack >Collection otherRack = > Iterables.getOnlyElement(validated.asMap().values()); > > return Lists.newArrayList(Iterables.limit(otherRack, 2)); > } > {code} > So with one or two racks we just return the first 2 entries in the list. > There's no shuffle or randomisation here. -- This message was sent by Atlassian JIRA (v6.4.14#64029) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-12884) Batch logic can lead to unbalanced use of system.batches
[ https://issues.apache.org/jira/browse/CASSANDRA-12884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16125719#comment-16125719 ] Daniel Cranford commented on CASSANDRA-12884: - Oh, and not to nitpick, but any reason to prefer {{otherRack.sublist(2, otherRack.size()).clear(); return otherRack();}} to {{return otherRack.sublist(0,2);}} ? > Batch logic can lead to unbalanced use of system.batches > > > Key: CASSANDRA-12884 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12884 > Project: Cassandra > Issue Type: Bug > Components: Core >Reporter: Adam Hattrell >Assignee: Daniel Cranford > Fix For: 3.0.x, 3.11.x > > Attachments: 0001-CASSANDRA-12884.patch > > > It looks as though there are some odd edge cases in how we distribute the > copies in system.batches. > The main issue is in the filter method for > org.apache.cassandra.batchlog.BatchlogManager > {code:java} > if (validated.size() - validated.get(localRack).size() >= 2) > { > // we have enough endpoints in other racks > validated.removeAll(localRack); > } > if (validated.keySet().size() == 1) > { >// we have only 1 `other` rack >Collection otherRack = > Iterables.getOnlyElement(validated.asMap().values()); > > return Lists.newArrayList(Iterables.limit(otherRack, 2)); > } > {code} > So with one or two racks we just return the first 2 entries in the list. > There's no shuffle or randomisation here. -- This message was sent by Atlassian JIRA (v6.4.14#64029) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-12884) Batch logic can lead to unbalanced use of system.batches
[ https://issues.apache.org/jira/browse/CASSANDRA-12884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16125710#comment-16125710 ] Daniel Cranford commented on CASSANDRA-12884: - I had originally considered using sublist to avoid creating a second ArrayList, but decided against it because the sublist version throws an exception in the degenerate case where there is only 1 element in otherRack. But now that I trace through the code, I think that otherRack is guaranteed to have at least 2 elements. If otherRack is the local rack and only has 1 element, {{if(validated.size() <= 2)}} would have been true, and the filter() function would have already returned. If otherRack was the single non-local rack, and had size 1, then {{if(validated.size() - validated.get(localRack).size() >= 2)}} would be false and the whole single-other-rack block wouldn't run. It's probably worth a comment stating that otherRack is guaranteed to have at least 2 elements. Looks good! > Batch logic can lead to unbalanced use of system.batches > > > Key: CASSANDRA-12884 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12884 > Project: Cassandra > Issue Type: Bug > Components: Core >Reporter: Adam Hattrell >Assignee: Daniel Cranford > Fix For: 3.0.x, 3.11.x > > Attachments: 0001-CASSANDRA-12884.patch > > > It looks as though there are some odd edge cases in how we distribute the > copies in system.batches. > The main issue is in the filter method for > org.apache.cassandra.batchlog.BatchlogManager > {code:java} > if (validated.size() - validated.get(localRack).size() >= 2) > { > // we have enough endpoints in other racks > validated.removeAll(localRack); > } > if (validated.keySet().size() == 1) > { >// we have only 1 `other` rack >Collection otherRack = > Iterables.getOnlyElement(validated.asMap().values()); > > return Lists.newArrayList(Iterables.limit(otherRack, 2)); > } > {code} > So with one or two racks we just return the first 2 entries in the list. > There's no shuffle or randomisation here. -- This message was sent by Atlassian JIRA (v6.4.14#64029) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-12884) Batch logic can lead to unbalanced use of system.batches
[ https://issues.apache.org/jira/browse/CASSANDRA-12884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16125688#comment-16125688 ] Aleksey Yeschenko commented on CASSANDRA-12884: --- The code is correct, but you could avoid creating an extra {{ArrayList}} there by clearing extra elements of the collection in-place. While we are here, can also simplify that slightly weird use of {{Iterables.getOnlyElement(validated.asMap().values())}} - in our case it's essentially equivalent to {{validated.values()}}. And a formatting nit: we put braces on new lines, always. Pushed a tiny commit on top with these suggestions addressed [here|https://github.com/iamaleksey/cassandra/commits/12884-3.0]. Does it look alright to you? > Batch logic can lead to unbalanced use of system.batches > > > Key: CASSANDRA-12884 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12884 > Project: Cassandra > Issue Type: Bug > Components: Core >Reporter: Adam Hattrell >Assignee: Daniel Cranford > Fix For: 3.0.x, 3.11.x > > Attachments: 0001-CASSANDRA-12884.patch > > > It looks as though there are some odd edge cases in how we distribute the > copies in system.batches. > The main issue is in the filter method for > org.apache.cassandra.batchlog.BatchlogManager > {code:java} > if (validated.size() - validated.get(localRack).size() >= 2) > { > // we have enough endpoints in other racks > validated.removeAll(localRack); > } > if (validated.keySet().size() == 1) > { >// we have only 1 `other` rack >Collection otherRack = > Iterables.getOnlyElement(validated.asMap().values()); > > return Lists.newArrayList(Iterables.limit(otherRack, 2)); > } > {code} > So with one or two racks we just return the first 2 entries in the list. > There's no shuffle or randomisation here. -- This message was sent by Atlassian JIRA (v6.4.14#64029) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-12884) Batch logic can lead to unbalanced use of system.batches
[ https://issues.apache.org/jira/browse/CASSANDRA-12884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16122335#comment-16122335 ] Daniel Cranford commented on CASSANDRA-12884: - Technically, if efficiency is key, we could implement something like a Durstenfeld/Knuth shuffle, eg https://stackoverflow.com/a/35278327 > Batch logic can lead to unbalanced use of system.batches > > > Key: CASSANDRA-12884 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12884 > Project: Cassandra > Issue Type: Bug > Components: Core >Reporter: Adam Hattrell >Assignee: Daniel Cranford > Fix For: 3.0.x, 3.11.x > > Attachments: 0001-CASSANDRA-12884.patch > > > It looks as though there are some odd edge cases in how we distribute the > copies in system.batches. > The main issue is in the filter method for > org.apache.cassandra.batchlog.BatchlogManager > {code:java} > if (validated.size() - validated.get(localRack).size() >= 2) > { > // we have enough endpoints in other racks > validated.removeAll(localRack); > } > if (validated.keySet().size() == 1) > { >// we have only 1 `other` rack >Collection otherRack = > Iterables.getOnlyElement(validated.asMap().values()); > > return Lists.newArrayList(Iterables.limit(otherRack, 2)); > } > {code} > So with one or two racks we just return the first 2 entries in the list. > There's no shuffle or randomisation here. -- This message was sent by Atlassian JIRA (v6.4.14#64029) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-12884) Batch logic can lead to unbalanced use of system.batches
[ https://issues.apache.org/jira/browse/CASSANDRA-12884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16122288#comment-16122288 ] Daniel Cranford commented on CASSANDRA-12884: - 1) BatchlogManager::shuffle is stubbed out so the unit test can provide a deterministic override. The unit test has been expanded to provide a test which catches this regression. (the existing code used the same pattern for getRandomInt which is overridden to be non-random in the unit test) 2) getRandomInt could return the same value twice (sampling with replacement) resulting in the same replica being chosen. The existing code uses the shuffle+take head pattern, eg in BatchlogManager.java line 545 {{shuffle((List) racks);}} and line 550 {{for (String rack : Iterables.limit(racks, 2))}} to perform sampling without replacement. > Batch logic can lead to unbalanced use of system.batches > > > Key: CASSANDRA-12884 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12884 > Project: Cassandra > Issue Type: Bug > Components: Core >Reporter: Adam Hattrell >Assignee: Daniel Cranford > Fix For: 3.0.x, 3.11.x > > Attachments: 0001-CASSANDRA-12884.patch > > > It looks as though there are some odd edge cases in how we distribute the > copies in system.batches. > The main issue is in the filter method for > org.apache.cassandra.batchlog.BatchlogManager > {code:java} > if (validated.size() - validated.get(localRack).size() >= 2) > { > // we have enough endpoints in other racks > validated.removeAll(localRack); > } > if (validated.keySet().size() == 1) > { >// we have only 1 `other` rack >Collection otherRack = > Iterables.getOnlyElement(validated.asMap().values()); > > return Lists.newArrayList(Iterables.limit(otherRack, 2)); > } > {code} > So with one or two racks we just return the first 2 entries in the list. > There's no shuffle or randomisation here. -- This message was sent by Atlassian JIRA (v6.4.14#64029) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-12884) Batch logic can lead to unbalanced use of system.batches
[ https://issues.apache.org/jira/browse/CASSANDRA-12884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=1614#comment-1614 ] Jeff Jirsa commented on CASSANDRA-12884: [~iamaleksey] will have a more comprehensive review, I'm sure, but a few notes from a very cursory glance: 1) I don't see the purpose of stubbing out {{BatchlogManager::shuffle}} as a helper function here. 2) In the case where {{validated.keySet().size() == 1}} , shuffling all of the IPs in a given rack may not be all that efficient - may be quicker to just pick 2 random ints, and grab the IPs at those offsets (like we do for the case where we have more than 2 racks, {{result.add(rackMembers.get(getRandomInt(rackMembers.size(;}} ) > Batch logic can lead to unbalanced use of system.batches > > > Key: CASSANDRA-12884 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12884 > Project: Cassandra > Issue Type: Bug > Components: Core >Reporter: Adam Hattrell >Assignee: Daniel Cranford > Fix For: 3.0.x, 3.11.x > > Attachments: 0001-CASSANDRA-12884.patch > > > It looks as though there are some odd edge cases in how we distribute the > copies in system.batches. > The main issue is in the filter method for > org.apache.cassandra.batchlog.BatchlogManager > {code:java} > if (validated.size() - validated.get(localRack).size() >= 2) > { > // we have enough endpoints in other racks > validated.removeAll(localRack); > } > if (validated.keySet().size() == 1) > { >// we have only 1 `other` rack >Collection otherRack = > Iterables.getOnlyElement(validated.asMap().values()); > > return Lists.newArrayList(Iterables.limit(otherRack, 2)); > } > {code} > So with one or two racks we just return the first 2 entries in the list. > There's no shuffle or randomisation here. -- This message was sent by Atlassian JIRA (v6.4.14#64029) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-12884) Batch logic can lead to unbalanced use of system.batches
[ https://issues.apache.org/jira/browse/CASSANDRA-12884?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16119980#comment-16119980 ] Daniel Cranford commented on CASSANDRA-12884: - Same bug. Regression. > Batch logic can lead to unbalanced use of system.batches > > > Key: CASSANDRA-12884 > URL: https://issues.apache.org/jira/browse/CASSANDRA-12884 > Project: Cassandra > Issue Type: Bug > Components: Core >Reporter: Adam Hattrell >Assignee: Joshua McKenzie > Fix For: 3.0.x, 3.11.x > > Attachments: 0001-CASSANDRA-12884.patch > > > It looks as though there are some odd edge cases in how we distribute the > copies in system.batches. > The main issue is in the filter method for > org.apache.cassandra.batchlog.BatchlogManager > {code:java} > if (validated.size() - validated.get(localRack).size() >= 2) > { > // we have enough endpoints in other racks > validated.removeAll(localRack); > } > if (validated.keySet().size() == 1) > { >// we have only 1 `other` rack >Collection otherRack = > Iterables.getOnlyElement(validated.asMap().values()); > > return Lists.newArrayList(Iterables.limit(otherRack, 2)); > } > {code} > So with one or two racks we just return the first 2 entries in the list. > There's no shuffle or randomisation here. -- This message was sent by Atlassian JIRA (v6.4.14#64029) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org