Added API and tests for CASSANDRA-10145 by beltran · Pull Request #823 · apache/cassandra-python-driver

beltran · 2017-09-11T18:39:15Z

No description provided.

beltran

The main concerns for me are:

Should we use ProtocolVersion.uses_keyspace_flag when building a BatchMessage, QueryMessage and PreparedMessage or not raise an exception from inside encoding?
Should we raise an exception if the use does something like session.execute(SimpleStatement("SELECT * from system.local", keyspace="system")), which would break the current API.
Should we add the parameter keyspace to sesion.prepare and session.prepare_on_all_hosts

beltran · 2017-09-11T18:40:35Z

                        if not success:
                            log.debug("Got unexpected response when preparing "
                                      "statement on host %s: %r", host, response)
+            """


This is my opinion where we get the biggest improvement, we don't have to set the keyspace of the connection, we can just send the query with the keyspace parameter, not sure how to add it though but a big if ProtocolVersion.uses_keyspace_flag(self._protocol_version)... else

Yeah, that's a cool side effect! I'm ok with a big if statement, or adding a new function to deal with keyspace-setting.

beltran · 2017-09-11T18:43:25Z


        if isinstance(query, SimpleStatement):
            query_string = query.query_string
+            statement_keyspace = query.keyspace


The reason we don't compare here if
keyspace = query.keyspace if ProtocolVersion.uses_keyspace_flag(self._protocol_version) else None
is because if the use creates a statement like
SimpleStatement("SELECT * from system.local".format(self.table_name), keyspace="system") I think a exception should be raised. This breakes the current API, since some applications doing this (may be bugged applications) will now raise an Exception if they are using a lower protocol.

beltran · 2017-09-11T18:44:36Z

                    "BatchStatement execution is only supported with protocol version "
                    "2 or higher (supported in Cassandra 2.0 and higher).  Consider "
                    "setting Cluster.protocol_version to 2 to support this operation.")
+            statement_keyspace = query.keyspace if ProtocolVersion.uses_keyspace_flag(self._protocol_version) else None


We need to compare here because a BoundStatment always has a keyspace set and this would raise an Exception under V5 protocol.

beltran · 2017-09-11T18:46:07Z

            log.exception("Error preparing query:")
            raise

+        prepared_keyspace = message.keyspace if message.keyspace else self.keyspace


This will always be self.keyspace since the message is built like message = PrepareMessage(query=query). We can add the keyspace parameter to session.prepare but that's probably a different ticket.

beltran · 2017-09-11T18:48:41Z



-def get_default_protocol():
+def set_default_beta_flag_true():


This set the flag allow_beta_protocol_version to True by default so we don't have to pass the argument. This is temporary since it looks like V5 won't be beta in Cassandra 4.0 although it is now.

beltran · 2017-09-11T18:51:09Z

        return prepared_statement

-    def prepare_on_all_hosts(self, query, excluded_host):
+    def prepare_on_all_hosts(self, query, excluded_host, keyspace=None):


Same as with prepare, should keyspace be added here as a parameter? I'm more inclined to add it here but maybe the best is to add it together with session.prepare since it's only called from inside that function.

beltran · 2017-09-11T18:53:03Z

+        # set on queries with protocol version 5 or higher. Consider setting Cluster.protocol_version to 5.',),
+        # <Host: 127.0.0.2 datacenter1>: ConnectionException('Host has been marked down or removed',),
+        # <Host: 127.0.0.1 datacenter1>: ConnectionException('Host has been marked down or removed',)})
+        with self.assertRaises(NoHostAvailable):


Should setting the keyspace in SimpleStatement raise UnsupportedOperation or UnsupportedOperation for old versions? As mentioned in a previous comment, raising something would break the API, since I believe now the keyspace is just ignored-

Ah, good point. For now, let's ignore it, but maybe in the next major we want to change that. Could you add a note to PYTHON-504?

beltran · 2017-09-11T20:33:00Z

Well, as for the second point, we are doing that in CQLEngine when we create SimpleStatement so I guess we shouldn't raise an exception and just ignore the keyspace as until now.

mambocab

Should we use ProtocolVersion.uses_keyspace_flag when building a BatchMessage, QueryMessageandPreparedMessage` or not raise an exception from inside encoding?

I'm not entirely sure what choice you mean here, but I think it'd be correct for the encoding step to ignore keyspace if it's set but unsupported in the current protocol version.

Should we raise an exception if the use does something like session.execute(SimpleStatement("SELECT * from system.local", keyspace="system")), which would break the current API.

I think I addressed this in my comments above -- I think we want to ignore stuff and not break the current API.

Should we add the parameter keyspace to sesion.prepare and session.prepare_on_all_hosts

I think we should -- the Java analogue takes a keyspace indirectly, but we can take it more directly.

mambocab · 2017-09-12T18:47:54Z

+        # set on queries with protocol version 5 or higher. Consider setting Cluster.protocol_version to 5.',),
+        # <Host: 127.0.0.2 datacenter1>: ConnectionException('Host has been marked down or removed',),
+        # <Host: 127.0.0.1 datacenter1>: ConnectionException('Host has been marked down or removed',)})
+        with self.assertRaises(NoHostAvailable):


Ah, good point. For now, let's ignore it, but maybe in the next major we want to change that. Could you add a note to PYTHON-504?

mambocab · 2017-09-12T18:51:02Z

                        if not success:
                            log.debug("Got unexpected response when preparing "
                                      "statement on host %s: %r", host, response)
+            """


Yeah, that's a cool side effect! I'm ok with a big if statement, or adding a new function to deal with keyspace-setting.

mambocab · 2017-09-12T19:06:50Z

+                    if not success:
+                        log.debug("Got unexpected response when preparing "
+                                  "statement on host %s: %r", host, response)
+            """


Sorry if I've missed something -- is this meant to be uncommented on merge?

mambocab

Small style change request for the send_chunks for sure -- and maybe we add docs for the new API?

mambocab · 2017-09-13T15:06:24Z

-                for i in range(0, len(ks_statements), 10):
-                    chunks.append(ks_statements[i:i + 10])
-
+            def send_chunks(chunks, set_keyspace=False):


I'd bring this out to a class-level function, but otherwise I love it

mambocab · 2017-09-13T15:06:41Z

+                chunks = []
+                for i in range(0, len(statements), 10):
+                    chunks.append(statements[i:i + 10])
+                    send_chunks(chunks, True)


You were right, this is totally sick

mambocab · 2017-09-13T15:45:49Z

            fn(response_future, *args, **kwargs)

-    def prepare(self, query, custom_payload=None):
+    def prepare(self, query, custom_payload=None, keyspace=None):


So the next question here is this: should we document the keyspace param here? or is this mostly intended for internal consumption? What does a user gain from using this parameter? I can see it either way, as it's functionally redundant with specifying the keyspace in your query string for many/most users.

I'm more inclined to document it, it shouldn't be long and it may be helpful for some users

mambocab · 2017-09-13T16:13:59Z


 @greaterthanorequalcass40
 class SimpleWithKeyspaceTests(QueryKeyspaceTests, unittest.TestCase):
+    @unittest.skip


Why are we skipping this now?

Specifying the keyspace before V5 is allowed right now and raising an exception would break the API. The keyspace is ignored bu there may be people doing it (like us in CQLEngine), I added that as part of PYTHON-504, we can remove the skip after that.

mambocab · 2017-09-14T14:56:41Z

I've expanded the new documentation a little -- other than that, looks ready to go!

PYTHON-678

beltran commented Sep 11, 2017

View reviewed changes

beltran requested a review from mambocab September 12, 2017 18:07

mambocab reviewed Sep 12, 2017

View reviewed changes

mambocab suggested changes Sep 13, 2017

View reviewed changes

Added API and tests for CASSANDRA-10145

e080510

PYTHON-678

mambocab force-pushed the python_CASSANDRA-10145-API branch from 1f6a338 to e080510 Compare September 14, 2017 16:24

mambocab approved these changes Sep 14, 2017

View reviewed changes

mambocab merged commit fd991d1 into python_CASSANDRA-10145 Sep 14, 2017

mambocab deleted the python_CASSANDRA-10145-API branch September 14, 2017 16:25

mambocab pushed a commit that referenced this pull request Sep 14, 2017

Added API and tests for CASSANDRA-10145 (#823)

81172ef

PYTHON-678



		def get_default_protocol():
		def set_default_beta_flag_true():

Conversation

beltran commented Sep 11, 2017

Uh oh!

beltran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beltran commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mambocab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mambocab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mambocab commented Sep 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

beltran commented Sep 11, 2017 •

edited

Loading