Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Fix Version/s: 0.7 beta 1
    • Component/s: Core
    • Labels:
      None

      Description

      We need the ability to manipulate the KS/CF definitions structures in DatabaseDescriptor.

        Activity

        Hide
        Hudson added a comment -

        Integrated in Cassandra #376 (See http://hudson.zones.apache.org/hudson/job/Cassandra/376/)
        improve concurrency during defn mutations. Patch by Gary Dusbabek, reviewed by Jonthan Ellis.
        rename a cf on a single node. Patch by Gary Dusbabek, reviewed by Jonthan Ellis.
        drop a cf on a single node. Patch by Gary Dusbabek, reviewed by Jonthan Ellis.

        Show
        Hudson added a comment - Integrated in Cassandra #376 (See http://hudson.zones.apache.org/hudson/job/Cassandra/376/ ) improve concurrency during defn mutations. Patch by Gary Dusbabek, reviewed by Jonthan Ellis. rename a cf on a single node. Patch by Gary Dusbabek, reviewed by Jonthan Ellis. drop a cf on a single node. Patch by Gary Dusbabek, reviewed by Jonthan Ellis.
        Hide
        Jonathan Ellis added a comment -

        +1

        Show
        Jonathan Ellis added a comment - +1
        Hide
        Gary Dusbabek added a comment -

        Addressed that problem in v3-0001.

        Show
        Gary Dusbabek added a comment - Addressed that problem in v3-0001.
        Hide
        Gary Dusbabek added a comment -

        I saw CLS but thought CFS. I'll see about putting a log message in the CL.

        Show
        Gary Dusbabek added a comment - I saw CLS but thought CFS. I'll see about putting a log message in the CL.
        Hide
        Jonathan Ellis added a comment -

        I was looking at this fragment

        + // reinitialize the table.
        + Table.open(ksm.name).addCf(cfm.cfName);
        + DatabaseDescriptor.setTableDefinition(ksm, newVersion);
        +
        + // force creation of a new commit log segment.
        + CommitLog.instance().forceNewSegment();

        isn't that saying the CLS happens afterwards, not before? (which only makes sense, since how can it know about the new entry until it's in the table / DD)

        Show
        Jonathan Ellis added a comment - I was looking at this fragment + // reinitialize the table. + Table.open(ksm.name).addCf(cfm.cfName); + DatabaseDescriptor.setTableDefinition(ksm, newVersion); + + // force creation of a new commit log segment. + CommitLog.instance().forceNewSegment(); isn't that saying the CLS happens afterwards, not before? (which only makes sense, since how can it know about the new entry until it's in the table / DD)
        Hide
        Gary Dusbabek added a comment -

        A new CF isn't considered to be added until the CLS is ready to go. And the table definitions aren't updated with the new cf until after that happens. So we are covered.

        Show
        Gary Dusbabek added a comment - A new CF isn't considered to be added until the CLS is ready to go. And the table definitions aren't updated with the new cf until after that happens. So we are covered.
        Hide
        Jonathan Ellis added a comment -

        does it tolerate the case of "cf is added, but CLS w/ the new entry is not created yet" w/o blowing up? (should just log No Such CF Yet or some such imo)

        Show
        Jonathan Ellis added a comment - does it tolerate the case of "cf is added, but CLS w/ the new entry is not created yet" w/o blowing up? (should just log No Such CF Yet or some such imo)
        Hide
        Gary Dusbabek added a comment -

        It took all my restraint to resist the urge to grab Table.flusherLock during the Table.[add|drop|rename]Cf() operations. Also, not included in this patchset, but in my branch, is a null check against the CFS during Table.apply().

        Show
        Gary Dusbabek added a comment - It took all my restraint to resist the urge to grab Table.flusherLock during the Table. [add|drop|rename] Cf() operations. Also, not included in this patchset, but in my branch, is a null check against the CFS during Table.apply().
        Hide
        Jonathan Ellis added a comment -

        > My idea was to lock a table during a definition mutation in order to prevent a row mutation from happening in progress. I still think we need this. And I think a per-table lock gives us more availability than a global table open lock.

        You're using a read/write lock already pre-Multilock (iirc), so the only time Multilock matters is allowing writes to continue in KS X while modifying KS Y. Modification is rare enough, and over fast enough, that this isn't worth adding complexity for, especially when it's complexity w/ a performance penalty.

        (Again, it's worth pointing out that using double-checked locking in Table.open completely bypasses the lock for existing Table objects.)

        Having talked about locking strategies I will now go back to arguing that this is a solution to the wrong question. (If you are on that page already I apologize for continuing to beat that horse.

        RM.apply is very very performance critical. I don't want to add extra locking there, especially not read/write lock which is more expensive.

        As long as we are corruption-safe in the pedantic concurrency sense we shouldn't worry about mutations during schema modification. (Hence, I suspect that we do need to single-thread these to avoid potential bugs w/ renames.) The only op that is potentially problematic is a CF or KS rename, where ops that come in during the process can apply to the "wrong" one. But the same thing will happen to ops against the new name that get sent a ms or two earlier no matter what you do locking-wise. In other words, the client needs to quiesce that target himself anyway, so us locking during the actual fraction of a ms for the change doesn't really buy him anything.

        Show
        Jonathan Ellis added a comment - > My idea was to lock a table during a definition mutation in order to prevent a row mutation from happening in progress. I still think we need this. And I think a per-table lock gives us more availability than a global table open lock. You're using a read/write lock already pre-Multilock (iirc), so the only time Multilock matters is allowing writes to continue in KS X while modifying KS Y. Modification is rare enough, and over fast enough, that this isn't worth adding complexity for, especially when it's complexity w/ a performance penalty. (Again, it's worth pointing out that using double-checked locking in Table.open completely bypasses the lock for existing Table objects.) Having talked about locking strategies I will now go back to arguing that this is a solution to the wrong question. (If you are on that page already I apologize for continuing to beat that horse. RM.apply is very very performance critical. I don't want to add extra locking there, especially not read/write lock which is more expensive. As long as we are corruption-safe in the pedantic concurrency sense we shouldn't worry about mutations during schema modification. (Hence, I suspect that we do need to single-thread these to avoid potential bugs w/ renames.) The only op that is potentially problematic is a CF or KS rename, where ops that come in during the process can apply to the "wrong" one. But the same thing will happen to ops against the new name that get sent a ms or two earlier no matter what you do locking-wise. In other words, the client needs to quiesce that target himself anyway, so us locking during the actual fraction of a ms for the change doesn't really buy him anything.
        Hide
        Gary Dusbabek added a comment -

        I had some more thoughts on this last night.

        >I do think that Multilock is definitely overkill.
        My idea was to lock a table during a definition mutation in order to prevent a row mutation from happening in progress. I still think we need this. And I think a per-table lock gives us more availability than a global table open lock.

        I took a look at the code and since all row mutations go through RowMutation.apply(), we're in good shape for this. I couldn't find any code that hangs on to Table objects and calls Table.apply() at various times (although nothing in the code prevents this).

        >If it's not let's see if we can come up with a way to use Concurrent operations instead of trying to lock everyone.
        This makes sense.

        Show
        Gary Dusbabek added a comment - I had some more thoughts on this last night. >I do think that Multilock is definitely overkill. My idea was to lock a table during a definition mutation in order to prevent a row mutation from happening in progress. I still think we need this. And I think a per-table lock gives us more availability than a global table open lock. I took a look at the code and since all row mutations go through RowMutation.apply(), we're in good shape for this. I couldn't find any code that hangs on to Table objects and calls Table.apply() at various times (although nothing in the code prevents this). >If it's not let's see if we can come up with a way to use Concurrent operations instead of trying to lock everyone. This makes sense.
        Hide
        Gary Dusbabek added a comment -

        >It's safe for other threads to be messing about with existing Table references during a mutate right?
        No. It's very unsafe. ThreadA calls Table.open(A), ThreadB drops a CF in A, ThreadA then tries to mutate rows on that CF. At some point ThreadB will cause that table to release() and its column families go away. I'm not sure what the right behavior is in this case (loud failure IMO). That table essentially becomes invalid and will be replaced in Table.instances with a new version.

        Is that a bad approach? Possibly. I was hoping to avoid having to discover all the states in CFS that I would have to account for when a CF def is mutated. CFS wasn't designed from that standpoint and I think getting it there would be an onerous task.

        I think a fair compromise would be to mark Table instances invalid when they are released and have them throw an exception in apply() to indicate the reference has been invalidated and can no longer be operated on (I'm thinking in the context of renames, which are the most complicated, but this approach makes the most sense in the context of deletes).

        Show
        Gary Dusbabek added a comment - >It's safe for other threads to be messing about with existing Table references during a mutate right? No. It's very unsafe. ThreadA calls Table.open(A), ThreadB drops a CF in A, ThreadA then tries to mutate rows on that CF. At some point ThreadB will cause that table to release() and its column families go away. I'm not sure what the right behavior is in this case (loud failure IMO). That table essentially becomes invalid and will be replaced in Table.instances with a new version. Is that a bad approach? Possibly. I was hoping to avoid having to discover all the states in CFS that I would have to account for when a CF def is mutated. CFS wasn't designed from that standpoint and I think getting it there would be an onerous task. I think a fair compromise would be to mark Table instances invalid when they are released and have them throw an exception in apply() to indicate the reference has been invalidated and can no longer be operated on (I'm thinking in the context of renames, which are the most complicated, but this approach makes the most sense in the context of deletes).
        Hide
        Jonathan Ellis added a comment -

        Okay. I don't know that it's worth using a re-entrant lock (since both read locking and write are Rare Events) but that's not a big deal. I do think that Multilock is definitely overkill.

        It's safe for other threads to be messing about with existing Table references during a mutate right? ("this doesn't help terribly much if you already have a table reference") Because using double-checked locking in Table.open means that most of the time that's what you're going to get.

        If it's not let's see if we can come up with a way to use Concurrent operations instead of trying to lock everyone. I think it's doable – if we move the modifications themselves to an executor queue, then you don't have to worry about them stomping on each other and you only have to make sure you don't expose changes to the rest of the systems via the CFS map until they are ready.

        Show
        Jonathan Ellis added a comment - Okay. I don't know that it's worth using a re-entrant lock (since both read locking and write are Rare Events) but that's not a big deal. I do think that Multilock is definitely overkill. It's safe for other threads to be messing about with existing Table references during a mutate right? ("this doesn't help terribly much if you already have a table reference") Because using double-checked locking in Table.open means that most of the time that's what you're going to get. If it's not let's see if we can come up with a way to use Concurrent operations instead of trying to lock everyone. I think it's doable – if we move the modifications themselves to an executor queue, then you don't have to worry about them stomping on each other and you only have to make sure you don't expose changes to the rest of the systems via the CFS map until they are ready.
        Hide
        Gary Dusbabek added a comment -

        >What problem is the table locking trying to solve?
        Two threads modifying a keyspace at the same time. The lock is held during the entire mutation, not just to get access to the Table object. I'm not opposed to removing the openLock from Table.open() for performance reasons, but I thought it made sense from a concurrency standpoint (we were synchronizing on Table.class anyway). We might wish to chose performance here and assume people deserve what they get when they attempt row mutations while a keyspace is in flux.)

        Show
        Gary Dusbabek added a comment - >What problem is the table locking trying to solve? Two threads modifying a keyspace at the same time. The lock is held during the entire mutation, not just to get access to the Table object. I'm not opposed to removing the openLock from Table.open() for performance reasons, but I thought it made sense from a concurrency standpoint (we were synchronizing on Table.class anyway). We might wish to chose performance here and assume people deserve what they get when they attempt row mutations while a keyspace is in flux.)
        Hide
        Jonathan Ellis added a comment -

        What problem is the table locking trying to solve? Just concurrent access to columnFamilyStores_? If it is that then using a NBHM instead should be cleaner. I've always intended to refactor CFS to have a Table reference rather than a Table String.

        Show
        Jonathan Ellis added a comment - What problem is the table locking trying to solve? Just concurrent access to columnFamilyStores_? If it is that then using a NBHM instead should be cleaner. I've always intended to refactor CFS to have a Table reference rather than a Table String.
        Hide
        Gary Dusbabek added a comment -

        Gives the ability to add/remove/rename CFs on a single node. A table_open lock was introduced to prevent tables from being opened during operations (I realize this doesn't help terribly much if you already have a table reference). The last patch includes an alternate approach that makes it so that only the table being operated on is locked during an operation.

        CF ids now live in two places: CFMetaData and CommitLogHeader. the ids in CLH are stored in serialized state and are only used during recovery.

        Show
        Gary Dusbabek added a comment - Gives the ability to add/remove/rename CFs on a single node. A table_open lock was introduced to prevent tables from being opened during operations (I realize this doesn't help terribly much if you already have a table reference). The last patch includes an alternate approach that makes it so that only the table being operated on is locked during an operation. CF ids now live in two places: CFMetaData and CommitLogHeader. the ids in CLH are stored in serialized state and are only used during recovery.

          People

          • Assignee:
            Gary Dusbabek
            Reporter:
            Gary Dusbabek
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development