[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=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=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=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=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=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=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=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=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
