Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#76] Protocol v3 & v4 support, serialization/deserialization rewrite using scodec #171

Merged
merged 58 commits into from
Jan 11, 2017
Merged

Conversation

tolbertam
Copy link
Contributor

@tolbertam tolbertam commented Aug 9, 2016

For #76, Adds protocol v3 and v4 support. This is a work in progress, but it's functional and currently all tests pass.

Summary of the major changes:

  • Use scodec for serialization/deserialization. All serde has been moved into a standalone codec module with scodec being the only dependency.
  • Change Prime store implementations to keep the request intact instead of deconstructing on input and reconstructing when receiving via the route APIs. This should keep things simpler. Added a memoized prime member on the Then implementations to convert the inputs to a Prime.

There should be no functional changes for users with one slight exception. Previously if no Prime is found for a query, query parameters would not be recorded in the activity log for batch and prepared statements. Now it records the query parameters as if they were varchars. There may also be some small nuances in conversions from json strings/numbers/booleans/etc. into the native type for a given data type, but i haven't identified any yet.

What is remaining:

  • Update cql-antlr to include the new types added in v3/v4 (tuples, udt, time, date, smallint, tinyint, etc.). The codecs exist for these, but need to be tested. I think there might be some barriers to supporting UDTs (covered in User defined type support #159), so depending on the difficulty level I may defer this. ([#3] Add support types added in protocol v3 and v4 cql-antlr#4)
  • Clean up, document and add more tests for the codec module. scodec is really nice, but its API is intimidating from my view. It is also not a complete implementation of the protocol, but the existing implementation isn't either (can maybe do more as follow ons).
  • Replace ServerStubrRunnerIntegrationTest / PrimingJsonHelperTest. I removed these earlier, need to replace them with new alternatives.
  • Add/update headers (pending Update license headers and add validation to 'check' #170 merge)
  • Clean up commit history

@chbatey
Copy link
Member

chbatey commented Sep 5, 2016

How's this going?

Are you at the Cassandra summit? Might be nice to review together

@tolbertam
Copy link
Contributor Author

tolbertam commented Sep 5, 2016

Hi @chbatey, it's going pretty well! Existing tests are passing, what's left is adding additional tests in the codec module, adding additional tests for the new types and maybe some clean up. Hopefully I can complete that really soon.

I'll be at the summit, I'd definitely be up for a review, that'd be fun!

@tolbertam
Copy link
Contributor Author

Rebased against master and added license headers. Working on finishing up tests in the codec module and updating rest of the code to support the v3/v4 types. I'll probably get that all going and clean up the commit history today.

@chbatey
Copy link
Member

chbatey commented Oct 11, 2016

Busy man!

@tolbertam
Copy link
Contributor Author

rebased on master to get the cql-antlr changes. I'll proceed to wire in the new types, and from there that should leave just some testing/doc updates/cleanup!

@tolbertam
Copy link
Contributor Author

Getting closer, completed adding the new types and failures. Going to spend some time tomorrow evening updating the docs. I'll then do some cleanup / optimization of the codec code and clean up the commit history. If all goes well should complete sometime this weekend.

Use 'AnyMatch' as catch all
Use terminate instead of shutdown.
Inline own implicit conversions.  One small difference between this
implementation and scodec-akka is this uses reflection instead of a
java class.
Have collection-based data types consider protocol version for length.
Batch considers protocol version for whether or not to include flags.
Prepare considers protocol version for whether or not to include pks.
native PF for encoding from Any -> Native type for data type.
Use CL from statement only if prime does not include one.
There are enough changes here, some of which may be (unintentionally)
breaking, bumping up the minor version seems like the right way to go
here.
@tolbertam
Copy link
Contributor Author

I've spent some time the last few days adding integration tests for the new types and errors, and some more tests and documentation for the codec module.

I think i've got everything worked out and this is ready aside from the commit history. @chbatey would you prefer I squash the commits now or would you have me leave it as is for now for review?

@tolbertam tolbertam changed the title [#76] [wip] Protocol v3 & v4 support, serialization/deserialization rewrite using scodec [#76] Protocol v3 & v4 support, serialization/deserialization rewrite using scodec Oct 16, 2016
@mgobec
Copy link

mgobec commented Oct 16, 2016

Great work @tolbertam 👍

@chbatey
Copy link
Member

chbatey commented Nov 18, 2016

Taking a look this weekend

spray-routing-shapeless2 depends on shapeless 2.1, where scodec
depends on shapeless 2.3.  Depending on what is being used for
dependency resolution, if shapeless 2.1 is chosen over 2.3 this causes
incompatibility issues with scodec.
private[this] def inspectProtocolVersion(input: ByteVector): Try[(ProtocolFlags, ByteVector)] = {
next[ProtocolFlags](input).flatMap {
case ((ProtocolFlags(_, UnsupportedProtocolVersion(v)), _)) =>
Failure(new UnsupportedProtocolException(v))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for new

val registerHandler = registerHandlerFactory(context)
write(Ready, frame.header)
context become initialized(queryHandler, batchHandler, registerHandler)
case _ => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove braces to make it consistent with the other cases

case result: FatalResult => result.produceFatalError(connection)
writePrime(execute, prime, header, Some(connection), alternative=Some(Reply(VoidResult)), consistency = Some(execute.parameters.consistency))
case None =>
var errMsg = s"Could not find prepared statement with id: 0x${execute.id.toHex}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be a val

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d'oh! I've been doing too much javascript lately 😄

writePrime(execute, prime, header, Some(connection), alternative=Some(Reply(VoidResult)), consistency = Some(execute.parameters.consistency))
case None =>
var errMsg = s"Could not find prepared statement with id: 0x${execute.id.toHex}"
activityLog.recordPreparedStatementExecution(errMsg, execute.parameters.consistency, Nil, Nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i take it you found this useful for testing? just a little off we use an error msg as the statement text

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrmm, I may be misunderstanding, isn't the behavior the same as before? I think the result is in this case that we record a prepared statement execution with the error message as the query text, and then an Unprepared message gets sent back to the client which will cause most drivers to repreare the statement.

@chbatey
Copy link
Member

chbatey commented Jan 5, 2017

Happy to merge this whenever. I'll be adding some features shortly

@tolbertam
Copy link
Contributor Author

Happy to merge this whenever. I'll be adding some features shortly

Excellent, thanks for the review! I'll address your feedback this morning.

@tolbertam
Copy link
Contributor Author

added commit for review feedback, thanks! Would you like me to do anything with the commit history, or would you just prefer to squash the whole thing on merge?

@chbatey
Copy link
Member

chbatey commented Jan 5, 2017

Squashing into one if find!

It has been a long time since I looked at Scala lol

@tolbertam
Copy link
Contributor Author

tolbertam commented Jan 5, 2017

Just remembered, when it comes time to do a release with these changes, there are a few activites:

  • Merge Add deprecation notice cql-antlr#7 to indicate that repository is deprecated in favor of cql-antlr being in this repository now.
  • Update gh-pages branch / push to read the docs since i made some doc updates.

@chbatey
Copy link
Member

chbatey commented Jan 6, 2017

Thanks @tolbertam

@tolbertam
Copy link
Contributor Author

@chbatey would you like me to merge and go through the process to release this? Can do it tomorrow if you like.

@chbatey
Copy link
Member

chbatey commented Jan 10, 2017 via email

@tolbertam
Copy link
Contributor Author

Awesome, thanks! Will take care of that this evening :)

@tolbertam tolbertam merged commit 41432b2 into scassandra:master Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants