Conversation
|
#305 needs to be merged before this one. |
750eda0 to
87ffbe5
Compare
|
We should also consider modifying |
|
Agreed. Do you want me to amend this PR or should I open a separate ticket? We need to decide which exceptions should be taken into account and which of them shouldn't. |
|
Let's do it on the same branch. |
There was a problem hiding this comment.
Maybe we should be smarter than this for BLOBs. If we get a 100K blob, we're going to convert it to a string at line 467, then find out that this string is too long and truncate it.
Instead, we should check the ByteBuffer's capacity first and, if it's too long, slice() the part we're going to print and pass that to format. The size of the string is 2 + 2 * sizeOfBuffer (the initial 2 is for the "0x" prefix).
This is less of an issue for other datatypes.
There was a problem hiding this comment.
Good catch. I will do that.
There was a problem hiding this comment.
As requested, the builder now takes a Cluster param in anticipation of future use.
There was a problem hiding this comment.
Doc needs update at line 38 for this
|
Excellent test coverage! I had a few comments about comments and added a few tests of my own, but this is looking pretty good! |
40d099e to
9912827
Compare
|
@tolbertam I had to do a rebase today and want to make sure I didn't miss any of your additions: I saw a new test in LatencyAwarePolicyTest so far. Is there anything else? |
|
Hi @adutra, the other test was QueryLoggerTest#should_log_counter_batch_statements. It looks like that shows up in the commits, so that's good :) |
|
Looks good to me! Would you mind amending the last commit to fit within the text restriction for the first line of the commit? (I think it's around 72 characters). Other than that everything looks great and I would say it's good to merge from my perspective! 👍 |
No description provided.