Uploaded image for project: 'Cassandra'
  1. Cassandra
  2. CASSANDRA-4155

Make possible to authenticate user when loading data to Cassandra with BulkRecordWriter.

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Fix Version/s: 1.1.1
    • Component/s: None
    • Labels:
      None

      Description

      I need to use the authorization feature (i.e. provided by SimpleAuthenticator and SimpleAuthority). The problem is that it's impossible now to pass the credentials (cassandra.output.keyspace.username and cassandra.output.keyspace.passwd) to org.apache.cassandra.hadoop.ConfigHelper because of no setters for these variables. Moreover, even if they could be passed, nothing will change because they are unused - ExternalClient class from org.apache.cassandra.hadoop.BulkRecordWriter is not making use of them; it's not even receiving them and no authorization is provided.

      The proposed improvement is to make it possible to authenticate user when loading data to Cassandra with BulkRecordWriter by adding appropriate setters to ConfigHelper and then passing credentials to ExternalClient class so it could use it for authorization request.

      I have created a patch for this which I attach.

      This improvement was made in the way that does not charm existing ExternalClient interface usage, but I think that it would be a bit nicer way to call the new constructor every time (optionally with username and password set to null) in this code and keeping the old one, instead of having and using two different constructors in two different cases in one method. However, it's my first patch for Cassandra, so I submit a less "agressive" one and I'm waiting for suggestions for to modify it

      1. trunk-4155.txt
        5 kB
        Michał Michalski
      2. trunk-4155-nicer-version.txt
        5 kB
        Michał Michalski
      3. trunk-4155-updated-1.txt
        5 kB
        Michał Michalski

        Activity

        Hide
        michalm Michał Michalski added a comment -

        Make possible to authenticate when using BulkRecordWriter.

        Show
        michalm Michał Michalski added a comment - Make possible to authenticate when using BulkRecordWriter.
        Hide
        michalm Michał Michalski added a comment -

        Added a patch that brings the mentioned improvement into life

        Show
        michalm Michał Michalski added a comment - Added a patch that brings the mentioned improvement into life
        Hide
        michalm Michał Michalski added a comment -

        And the nicer version I mentioned.

        Show
        michalm Michał Michalski added a comment - And the nicer version I mentioned.
        Hide
        brandon.williams Brandon Williams added a comment -

        Thanks for the patch, Michal! I'm fine with the 'nicer' version, since the only place ExternalClient is instantiated is in BOF and multiple constructors with logic around which one to use starts getting messy, but let's go ahead and remove the old constructor (or rather, change the signature of the existing one depending on how you look at it) since we don't need it anymore.

        Show
        brandon.williams Brandon Williams added a comment - Thanks for the patch, Michal! I'm fine with the 'nicer' version, since the only place ExternalClient is instantiated is in BOF and multiple constructors with logic around which one to use starts getting messy, but let's go ahead and remove the old constructor (or rather, change the signature of the existing one depending on how you look at it) since we don't need it anymore.
        Hide
        michalm Michał Michalski added a comment -

        Thanks for quick response and review, Brandon! I like your idea so I'll modify the patch this way. It can be even done better - as ConfigHelper (of Configuration, to be more precise) returns null if value was not set, I don't have to do any "if" in BulkRecordWriter.prepareWriter() - I can just retrieve the values from config (null if not set) and pass them, so the only "if" will be in ExternalClient.init().

        I'll change it and submit a new patch as soon as I get my Hadoop-to-Cassandra bulkload work once again, as I suffer some "(...)ib-1-Data.db is not compatible with current version hc" problem after moving to RC1, so I could test it, just to be 100% sure it's OK

        Show
        michalm Michał Michalski added a comment - Thanks for quick response and review, Brandon! I like your idea so I'll modify the patch this way. It can be even done better - as ConfigHelper (of Configuration, to be more precise) returns null if value was not set, I don't have to do any "if" in BulkRecordWriter.prepareWriter() - I can just retrieve the values from config (null if not set) and pass them, so the only "if" will be in ExternalClient.init(). I'll change it and submit a new patch as soon as I get my Hadoop-to-Cassandra bulkload work once again, as I suffer some "(...)ib-1-Data.db is not compatible with current version hc" problem after moving to RC1, so I could test it, just to be 100% sure it's OK
        Hide
        michalm Michał Michalski added a comment -

        Updated version. One constructor with updated signature, removed unnecessary "if" statement.

        Show
        michalm Michał Michalski added a comment - Updated version. One constructor with updated signature, removed unnecessary "if" statement.
        Hide
        brandon.williams Brandon Williams added a comment -

        Committed, thanks!

        Show
        brandon.williams Brandon Williams added a comment - Committed, thanks!
        Hide
        michalm Michał Michalski added a comment -

        Not sure if it's the best place to report this, but I can see that the last changes in ./src/java/org/apache/cassandra/hadoop/ConfigHelper.java did some mess in this task. 09b4e7bdfb113715b3e1ae0be179415fab79d4b7 has removed all the setters and getters I added here and 65d51bb747cc5b7f60a58793026529cb4c6bdf1d has only restored one of them:

        michal@aperture:~/workspace/cassandra-trunk$ cat missing-code.tmp 
        commit 65d51bb747cc5b7f60a58793026529cb4c6bdf1d
        Author: Brandon Williams <brandonwilliams@apache.org>
        Date:   Wed Apr 18 14:44:58 2012 -0500
        
            Fix the fix of the crazy automerge
        
        diff --git a/src/java/org/apache/cassandra/hadoop/ConfigHelper.java b/src/java/org/apache/cassandra/hadoop/ConfigHelper.java
        index 81dbf38..8ec215e 100644
        --- a/src/java/org/apache/cassandra/hadoop/ConfigHelper.java
        +++ b/src/java/org/apache/cassandra/hadoop/ConfigHelper.java
        @@ -304,6 +304,11 @@ public class ConfigHelper
                 return conf.get(OUTPUT_KEYSPACE_USERNAME_CONFIG);
             }
         
        +    public static String getOutputKeyspacePassword(Configuration conf)
        +    {
        +        return conf.get(OUTPUT_KEYSPACE_PASSWD_CONFIG);
        +    }
        +
             public static String getInputColumnFamily(Configuration conf)
             {
                 return conf.get(INPUT_COLUMNFAMILY_CONFIG);
        
        commit 09b4e7bdfb113715b3e1ae0be179415fab79d4b7
        Author: Brandon Williams <brandonwilliams@apache.org>
        Date:   Wed Apr 18 14:29:38 2012 -0500
        
            Fix crazy automerge
        
        diff --git a/src/java/org/apache/cassandra/hadoop/ConfigHelper.java b/src/java/org/apache/cassandra/hadoop/ConfigHelper.java
        index 68a8761..81dbf38 100644
        --- a/src/java/org/apache/cassandra/hadoop/ConfigHelper.java
        +++ b/src/java/org/apache/cassandra/hadoop/ConfigHelper.java
        @@ -299,36 +299,11 @@ public class ConfigHelper
                 return conf.get(INPUT_KEYSPACE_PASSWD_CONFIG);
             }
         
        -    public static void setOutputKeyspaceUserName(Configuration conf, String username)
        -    {
        -        conf.set(OUTPUT_KEYSPACE_USERNAME_CONFIG, username);
        -    }
        -
        -    public static void setOutputKeyspaceUserName(Configuration conf, String username)
        -    {
        -        conf.set(OUTPUT_KEYSPACE_USERNAME_CONFIG, username);
        -    }
        -
             public static String getOutputKeyspaceUserName(Configuration conf)
             {
                 return conf.get(OUTPUT_KEYSPACE_USERNAME_CONFIG);
             }
         
        -    public static void setOutputKeyspacePassword(Configuration conf, String password)
        -    {
        -        conf.set(OUTPUT_KEYSPACE_PASSWD_CONFIG, password);
        -    }
        -
        -    public static void setOutputKeyspacePassword(Configuration conf, String password)
        -    {
        -        conf.set(OUTPUT_KEYSPACE_PASSWD_CONFIG, password);
        -    }
        -
        -    public static String getOutputKeyspacePassword(Configuration conf)
        -    {
        -        return conf.get(OUTPUT_KEYSPACE_PASSWD_CONFIG);
        -    }
        -
             public static String getInputColumnFamily(Configuration conf)
             {
                 return conf.get(INPUT_COLUMNFAMILY_CONFIG);
        

        Can you fix this or maybe should I prepare another patch against current trunk for this? Or maybe do it in separate issue?

        It really was a "crazy automerge"

        Show
        michalm Michał Michalski added a comment - Not sure if it's the best place to report this, but I can see that the last changes in ./src/java/org/apache/cassandra/hadoop/ConfigHelper.java did some mess in this task. 09b4e7bdfb113715b3e1ae0be179415fab79d4b7 has removed all the setters and getters I added here and 65d51bb747cc5b7f60a58793026529cb4c6bdf1d has only restored one of them: michal@aperture:~/workspace/cassandra-trunk$ cat missing-code.tmp commit 65d51bb747cc5b7f60a58793026529cb4c6bdf1d Author: Brandon Williams <brandonwilliams@apache.org> Date: Wed Apr 18 14:44:58 2012 -0500 Fix the fix of the crazy automerge diff --git a/src/java/org/apache/cassandra/hadoop/ConfigHelper.java b/src/java/org/apache/cassandra/hadoop/ConfigHelper.java index 81dbf38..8ec215e 100644 --- a/src/java/org/apache/cassandra/hadoop/ConfigHelper.java +++ b/src/java/org/apache/cassandra/hadoop/ConfigHelper.java @@ -304,6 +304,11 @@ public class ConfigHelper return conf.get(OUTPUT_KEYSPACE_USERNAME_CONFIG); } + public static String getOutputKeyspacePassword(Configuration conf) + { + return conf.get(OUTPUT_KEYSPACE_PASSWD_CONFIG); + } + public static String getInputColumnFamily(Configuration conf) { return conf.get(INPUT_COLUMNFAMILY_CONFIG); commit 09b4e7bdfb113715b3e1ae0be179415fab79d4b7 Author: Brandon Williams <brandonwilliams@apache.org> Date: Wed Apr 18 14:29:38 2012 -0500 Fix crazy automerge diff --git a/src/java/org/apache/cassandra/hadoop/ConfigHelper.java b/src/java/org/apache/cassandra/hadoop/ConfigHelper.java index 68a8761..81dbf38 100644 --- a/src/java/org/apache/cassandra/hadoop/ConfigHelper.java +++ b/src/java/org/apache/cassandra/hadoop/ConfigHelper.java @@ -299,36 +299,11 @@ public class ConfigHelper return conf.get(INPUT_KEYSPACE_PASSWD_CONFIG); } - public static void setOutputKeyspaceUserName(Configuration conf, String username) - { - conf.set(OUTPUT_KEYSPACE_USERNAME_CONFIG, username); - } - - public static void setOutputKeyspaceUserName(Configuration conf, String username) - { - conf.set(OUTPUT_KEYSPACE_USERNAME_CONFIG, username); - } - public static String getOutputKeyspaceUserName(Configuration conf) { return conf.get(OUTPUT_KEYSPACE_USERNAME_CONFIG); } - public static void setOutputKeyspacePassword(Configuration conf, String password) - { - conf.set(OUTPUT_KEYSPACE_PASSWD_CONFIG, password); - } - - public static void setOutputKeyspacePassword(Configuration conf, String password) - { - conf.set(OUTPUT_KEYSPACE_PASSWD_CONFIG, password); - } - - public static String getOutputKeyspacePassword(Configuration conf) - { - return conf.get(OUTPUT_KEYSPACE_PASSWD_CONFIG); - } - public static String getInputColumnFamily(Configuration conf) { return conf.get(INPUT_COLUMNFAMILY_CONFIG); Can you fix this or maybe should I prepare another patch against current trunk for this? Or maybe do it in separate issue? It really was a "crazy automerge"
        Hide
        brandon.williams Brandon Williams added a comment -

        Should be fixed on trunk in dbecb3023c7f660f0c5d8245820cdc74b93dbbf

        Show
        brandon.williams Brandon Williams added a comment - Should be fixed on trunk in dbecb3023c7f660f0c5d8245820cdc74b93dbbf

          People

          • Assignee:
            michalm Michał Michalski
            Reporter:
            michalm Michał Michalski
            Reviewer:
            Brandon Williams
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development