You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by belliottsmith <gi...@git.apache.org> on 2018/09/18 15:20:07 UTC

[GitHub] cassandra pull request #270: 14759

GitHub user belliottsmith opened a pull request:

    https://github.com/apache/cassandra/pull/270

    14759

    

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/belliottsmith/cassandra 14759

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/cassandra/pull/270.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #270
    
----
commit 72791132da319188e709e1177817f08c41d9d8c6
Author: Benedict Elliott Smith <be...@...>
Date:   2018-09-09T22:53:07Z

    Transient->Full movements mishandle consistency level upgrade
    
    patch by Benedict; reviewed by Alex Petrov and Ariel Weisberg for CASSANDRA-14759

commit 4011548a6bf5fd0722f6036c0c8d88b626ad25fc
Author: Benedict Elliott Smith <be...@...>
Date:   2018-09-18T15:18:37Z

    circleci

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


[GitHub] cassandra pull request #270: 14759

Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:

    https://github.com/apache/cassandra/pull/270#discussion_r218483495
  
    --- Diff: src/java/org/apache/cassandra/locator/ReplicaLayout.java ---
    @@ -275,7 +275,24 @@ public ForTokenWrite(EndpointsForToken natural, EndpointsForToken pending, Endpo
          */
         private static <E extends Endpoints<E>> E resolveWriteConflictsInNatural(E natural, E pending)
         {
    -        return natural.filter(r -> !r.isTransient() || !pending.contains(r.endpoint(), true));
    +        ReplicaCollection.Mutable<E> resolved = natural.newMutable(natural.size());
    +        for (Replica replica : natural)
    +        {
    +            // always prefer the full natural replica, if there is a conflict
    +            if (!replica.isFull())
    +            {
    +                Replica conflict = pending.byEndpoint().get(replica.endpoint());
    +                if (conflict != null)
    +                {
    +                    // If we have any pending transient->full movement, we need to move the full replica to our 'natural' bucket
    +                    // to avoid corrupting our count
    +                    if (conflict.isFull()) resolved.add(new Replica(replica.endpoint(), replica.range(), true));
    --- End diff --
    
    Why do you have to construct a new replica? You can't add conflict?
    
    Can conflict have a different range?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org


[GitHub] cassandra pull request #270: 14759

Posted by belliottsmith <gi...@git.apache.org>.
Github user belliottsmith commented on a diff in the pull request:

    https://github.com/apache/cassandra/pull/270#discussion_r218488201
  
    --- Diff: src/java/org/apache/cassandra/locator/ReplicaLayout.java ---
    @@ -275,7 +275,24 @@ public ForTokenWrite(EndpointsForToken natural, EndpointsForToken pending, Endpo
          */
         private static <E extends Endpoints<E>> E resolveWriteConflictsInNatural(E natural, E pending)
         {
    -        return natural.filter(r -> !r.isTransient() || !pending.contains(r.endpoint(), true));
    +        ReplicaCollection.Mutable<E> resolved = natural.newMutable(natural.size());
    +        for (Replica replica : natural)
    +        {
    +            // always prefer the full natural replica, if there is a conflict
    +            if (!replica.isFull())
    +            {
    +                Replica conflict = pending.byEndpoint().get(replica.endpoint());
    +                if (conflict != null)
    +                {
    +                    // If we have any pending transient->full movement, we need to move the full replica to our 'natural' bucket
    +                    // to avoid corrupting our count
    +                    if (conflict.isFull()) resolved.add(new Replica(replica.endpoint(), replica.range(), true));
    --- End diff --
    
    No, you're right, good catch.  Even if they are a different range, either should be acceptable for this query (by definition).  I'll push an update.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org