Skip to content

JAVA-1751: Include defaultTimestamp length in encodedSize for >= V3#950

Merged
adutra merged 5 commits into
3.xfrom
java1751
Feb 28, 2018
Merged

JAVA-1751: Include defaultTimestamp length in encodedSize for >= V3#950
adutra merged 5 commits into
3.xfrom
java1751

Conversation

@tolbertam
Copy link
Copy Markdown
Contributor

@tolbertam tolbertam commented Feb 14, 2018 •
edited
Loading

QueryProtocolOptions.encodedSize previously incorrectly only accounted
for defaultTimestamp length if protocol version was equal to V3. It
should be included for all protocol versions >= V3.

@tolbertam tolbertam added this to the 3.5.0 milestone Feb 14, 2018
@tolbertam
JAVA-1751: Include defaultTimestamp length in encodedSize for >= V3
84142d5 
QueryProtocolOptions.encodedSize previously incorrectly only accounted
for defaultTimestamp length if protocol version was equal to V3.  It
should be included for all protocol versions >= V3.
@olim7t
Copy link
Copy Markdown
Contributor

olim7t commented Feb 27, 2018

If there are other bugs in encodedSize, we want to fix them in order to allocate the optimal size and avoid resizing.
However blowing up if the size is wrong is probably too aggressive, at least for driver 3, since the error is recoverable. So I suggest adding a warning that will alert us if this happens elsewhere (it's the only warn log in this class, so easy to disable). I'm going to trigger our integration suite with it to check that it doesn't pop up.

@tolbertam
Copy link
Copy Markdown
Contributor Author

tolbertam commented Feb 27, 2018

👍 on adding the warning, thanks!

@tolbertam
Copy link
Copy Markdown
Contributor Author

tolbertam commented Feb 27, 2018

The warning identified at least one other issue, JAVA-1770 which I pushed a fix for in a89b30a. Will run through suite again to see if any other issues are identified.

tolbertam
tolbertam commented Feb 27, 2018
View reviewed changes
Comment thread driver-core/src/main/java/com/datastax/driver/core/CBUtil.java
cb.writeBytes(bytes);
}

public static void writeBytes(ByteBuffer bytes, ByteBuf cb) {
Copy link
Copy Markdown
Contributor Author

@tolbertam tolbertam Feb 27, 2018

Choose a reason for hiding this comment

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

Removed unused methods, while they might be useful in future, would be easy to reimplement them.

@adutra adutra merged commit 56e8009 into 3.x Feb 28, 2018
@adutra adutra deleted the java1751 branch February 28, 2018 14:06
@tolbertam
Copy link
Copy Markdown
Contributor Author

tolbertam commented Feb 28, 2018

I don't see any more incidences of the added warning, so I think we got them all 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@olim7t olim7t olim7t approved these changes

@newkek newkek newkek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.5.0

Development

Successfully merging this pull request may close these issues.

4 participants

@tolbertam @olim7t @newkek @adutra