Cassandra
  1. Cassandra
  2. CASSANDRA-1377

NPE aborts streaming operations for keyspaces with hyphens ('-') in their names

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 0.6.5, 0.7 beta 2
    • Component/s: Core
    • Labels:
      None

      Description

      When streaming starts for operations such as repair or bootstrap, it will fail due to an NPE if they rows are in a keyspace that has a hyphen in its name. One workaround for this issue would be to not use keyspace names containing hyphens. It would be even nicer if streaming worked for keyspace names with hyphens, since keyspaces named like that seem to be fine in all other ways.

      To reproduce:
      1. With a multi-node ring, load up a keyspace with a hyphen in its name
      2. Add some data to that keyspace
      3. nodetool repair

      Expected results:
      Repair operations complete normally

      Actual results:
      Repair operations don't complete normally. The stacktrace below is correlated with the repair request.

      INFO [AE-SERVICE-STAGE:1] 2010-06-30 14:11:29,744 AntiEntropyService.java (line 619) Performing streaming repair of 1 ranges to /10.255.0.20 for (my-keyspace,AColumnFamily)
      ERROR [MESSAGE-DESERIALIZER-POOL:1] 2010-06-30 14:11:30,034 DebuggableThreadPoolExecutor.java (line 101) Error in ThreadPoolExecutor
      java.lang.NullPointerException
      at org.apache.cassandra.streaming.StreamInitiateVerbHandler.getNewNames(StreamInitiateVerbHandler.java:154)
      at org.apache.cassandra.streaming.StreamInitiateVerbHandler.doVerb(StreamInitiateVerbHandler.java:76)
      at org.apache.cassandra.net.MessageDeliveryTask.run(MessageDeliveryTask.java:40)
      at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
      at java.lang.Thread.run(Thread.java:619)

      1. 1377-0.6-check-for-dashes.txt
        0.8 kB
        Gary Dusbabek
      2. CAS-1377-2.patch
        0.8 kB
        Erik Onnen
      3. CAS-1377-1.patch
        0.7 kB
        Erik Onnen
      4. CAS-1377.patch
        2 kB
        Erik Onnen
      5. 1377-0.6.txt
        1 kB
        Gary Dusbabek
      6. ASF.LICENSE.NOT.GRANTED--v1-0001-disallow-invalid-ks-cf-names.txt
        10 kB
        Gary Dusbabek

        Activity

        Hide
        Gary Dusbabek added a comment -

        We currently disallow hyphens in CF names. It sounds like we need to disallow it in KS names too.

        Show
        Gary Dusbabek added a comment - We currently disallow hyphens in CF names. It sounds like we need to disallow it in KS names too.
        Hide
        Jonathan Ellis added a comment -

        I vote for disallowing everything but \w characters - [a-zA-Z0-9_]

        Show
        Jonathan Ellis added a comment - I vote for disallowing everything but \w characters - [a-zA-Z0-9_]
        Hide
        Gary Dusbabek added a comment -

        0001 is for trunk, 1377-0.6 is for 0.6.

        Show
        Gary Dusbabek added a comment - 0001 is for trunk, 1377-0.6 is for 0.6.
        Hide
        Jonathan Ellis added a comment -

        +1

        Show
        Jonathan Ellis added a comment - +1
        Hide
        Hudson added a comment -

        Integrated in Cassandra #514 (See http://hudson.zones.apache.org/hudson/job/Cassandra/514/)
        disallow invalid ks+cf names. patch by gdusbabek, reviewed by jbellis. CASSANDRA-1377

        Show
        Hudson added a comment - Integrated in Cassandra #514 (See http://hudson.zones.apache.org/hudson/job/Cassandra/514/ ) disallow invalid ks+cf names. patch by gdusbabek, reviewed by jbellis. CASSANDRA-1377
        Hide
        Hudson added a comment -

        Integrated in Cassandra #518 (See https://hudson.apache.org/hudson/job/Cassandra/518/)
        CHANGES.txt and NEWS.txt update explaining the ramifications of CASSANDRA-1377

        Show
        Hudson added a comment - Integrated in Cassandra #518 (See https://hudson.apache.org/hudson/job/Cassandra/518/ ) CHANGES.txt and NEWS.txt update explaining the ramifications of CASSANDRA-1377
        Hide
        Gary Dusbabek added a comment -

        eonnen in #cassandra has made a compelling argument that committing this to 0.6 breaks some backwards compatibility. I am inclined to agree if the amount of work to handle dashes during streaming is trivial.

        Show
        Gary Dusbabek added a comment - eonnen in #cassandra has made a compelling argument that committing this to 0.6 breaks some backwards compatibility. I am inclined to agree if the amount of work to handle dashes during streaming is trivial.
        Hide
        Jonathan Ellis added a comment -

        We really need some "reserved" characters in keyspace/CF names. You can make a case that restricting them to \w is going too far in the other direction, but hyphens have always been reserved, and letting them pass in keyspaces was definitely a bug that was going to bite us (see: this issue).

        Show
        Jonathan Ellis added a comment - We really need some "reserved" characters in keyspace/CF names. You can make a case that restricting them to \w is going too far in the other direction, but hyphens have always been reserved, and letting them pass in keyspaces was definitely a bug that was going to bite us (see: this issue).
        Hide
        Jonathan Ellis added a comment -

        (One of the reasons I'd like to restrict to \w is that makes us not have to deal with people reporting bugs from Thrift strings being utf8-encoded in some languages and not in others.)

        Show
        Jonathan Ellis added a comment - (One of the reasons I'd like to restrict to \w is that makes us not have to deal with people reporting bugs from Thrift strings being utf8-encoded in some languages and not in others.)
        Hide
        Erik Onnen added a comment -

        We have a multi-tenant deployment hosting multiple customers, each with multiple deployments of our upstream software (think test/prod). For each customer deployment, we've had a UUID identifying the instance since before the dawn of time, or at least since before Cassandra

        Up until this change, using a keyspace-UUID mapping worked perfectly, especially after set_keyspace was added to the lower-level client API which allowed us to have pools for a given customer with different customers able to have different throughput to a point.

        In hopes of getting this relaxed just a bit to allow "-", I've attached a fix for the bug in 0.6.0 tested with a keyspace name of "e610eed7-c6be-449b-ad2c-562f35d75528" which is a Type4 UUID. If you can broaden the limit here just a bit, we'll be good. I don't think it's all that unrealistic that users will want to have a keyspace correspond to real artifacts in their system (although I don't feel the same about CF names). This would at least broaden things just enough to allow UUIDs at that level.

        While I understand the need to limit what goes into the name of the keyspace and why, it's too restrictive for my needs and I'll argue against it at least and try and patch my way out of it as long as you'll listen.

        Happy to do the same for 0.7.0 if you're receptive.

        Show
        Erik Onnen added a comment - We have a multi-tenant deployment hosting multiple customers, each with multiple deployments of our upstream software (think test/prod). For each customer deployment, we've had a UUID identifying the instance since before the dawn of time, or at least since before Cassandra Up until this change, using a keyspace-UUID mapping worked perfectly, especially after set_keyspace was added to the lower-level client API which allowed us to have pools for a given customer with different customers able to have different throughput to a point. In hopes of getting this relaxed just a bit to allow "-", I've attached a fix for the bug in 0.6.0 tested with a keyspace name of "e610eed7-c6be-449b-ad2c-562f35d75528" which is a Type4 UUID. If you can broaden the limit here just a bit, we'll be good. I don't think it's all that unrealistic that users will want to have a keyspace correspond to real artifacts in their system (although I don't feel the same about CF names). This would at least broaden things just enough to allow UUIDs at that level. While I understand the need to limit what goes into the name of the keyspace and why, it's too restrictive for my needs and I'll argue against it at least and try and patch my way out of it as long as you'll listen. Happy to do the same for 0.7.0 if you're receptive.
        Hide
        Erik Onnen added a comment -

        Fixes NPE with "-" in Keyspace names, re-allows using them at the DatabaseDescriptor.

        Show
        Erik Onnen added a comment - Fixes NPE with "-" in Keyspace names, re-allows using them at the DatabaseDescriptor.
        Hide
        Jonathan Ellis added a comment -

        committed Erik's patch to 0.6 and trunk.

        I still think we need to make some explicit rules about reserved characters but I agree that 0.6.5 is not the place to make that change.

        Show
        Jonathan Ellis added a comment - committed Erik's patch to 0.6 and trunk. I still think we need to make some explicit rules about reserved characters but I agree that 0.6.5 is not the place to make that change.
        Hide
        Erik Onnen added a comment -

        This patch lightens the restriction on KS names to allow "-" in addition to
        w bringing in line the functionality w/ the 0.6 patch submitted earlier.

        Show
        Erik Onnen added a comment - This patch lightens the restriction on KS names to allow "-" in addition to w bringing in line the functionality w/ the 0.6 patch submitted earlier.
        Hide
        Erik Onnen added a comment -

        Thanks Jonathan. I added one additional patch to relax things slightly in 0.7 trunk. Tested with a nodetool loadbalance and nodetool move with a two node ring and both worked fine. Lots of changes in the streaming code between 0.6 and 0.7 and it looks like the broken code didn't move forward into 0.7 near as I can tell in reading though it.

        Show
        Erik Onnen added a comment - Thanks Jonathan. I added one additional patch to relax things slightly in 0.7 trunk. Tested with a nodetool loadbalance and nodetool move with a two node ring and both worked fine. Lots of changes in the streaming code between 0.6 and 0.7 and it looks like the broken code didn't move forward into 0.7 near as I can tell in reading though it.
        Hide
        Erik Onnen added a comment -

        Forgot to attach test in CAS-1377-1.patch

        Show
        Erik Onnen added a comment - Forgot to attach test in CAS-1377-1.patch
        Hide
        Gary Dusbabek added a comment -

        Puts hyphen check back into CF definition.

        Show
        Gary Dusbabek added a comment - Puts hyphen check back into CF definition.
        Hide
        Jonathan Ellis added a comment -

        +1 hyphens-in-CF check

        Show
        Jonathan Ellis added a comment - +1 hyphens-in-CF check
        Hide
        Gary Dusbabek added a comment -

        committed.

        Show
        Gary Dusbabek added a comment - committed.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development