CASSANDRA-21417: Fix non-printable characters in Gossiper log for TOKENS - #5124
CASSANDRA-21417: Fix non-printable characters in Gossiper log for TOKENS#5124Suhel0328 wants to merge 4 commits into
Conversation
| * never meant to be printed as text - doing so produces unreadable, control-character-laden output | ||
| * (see CASSANDRA-21417). All other states are rendered normally. | ||
| */ | ||
| private static String appStateMapToString(Map<ApplicationState, VersionedValue> applicationState) |
There was a problem hiding this comment.
how about renaming to formatAppStateMapForLogging for a showing an explicit purpose?
| else | ||
| sb.append(entry.getValue()); | ||
| } | ||
| return sb.append('}').toString(); |
There was a problem hiding this comment.
wondering if we can use Streams API here.
| * TokenSerializer attempt a multi-gigabyte array allocation (see CASSANDRA-21417 discussion). | ||
| */ | ||
| @Test | ||
| public void testToStringHandlesUndecodableTokensValue() |
There was a problem hiding this comment.
is this function's name self-explanatory? does it need the above comment?
| * the raw bytes losslessly, and toString() must not print that raw data (see CASSANDRA-21417). | ||
| */ | ||
| @Test | ||
| public void testToStringDoesNotLeakRawTokenBytes() |
There was a problem hiding this comment.
how about renaming to testToStringSummarizesTokensInsteadOfPrintingRawBinaryData, is it self-containing?
|
@Suhel0328 can you put a picture of the logs verifying the change on ticket? |
|
@Suhel0328 make the status of ticket to "Patch Available" by "Open Issue" -> "Patch Available". |
| assertTrue(rendered.contains("RELEASE_VERSION=Value(")); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The current tests via EndpointState.toString() are correct, but I'd recommend extracting the app state formatting logic into a package-private method with dedicated tests.
- we are testing
toString()which includes heartbeat state, alive status, etc. - if toString() formatting changes for heartbeat or other fields, these tests might break even though the app state logic is fine
- test edge cases for appStateMapToString alone
|
Hi @AshenScribe, done changes as per the suggestion |
| VersionedValue value = entry.getValue(); | ||
| try | ||
| { | ||
| int numTokens = TokenSerializer.deserialize(DatabaseDescriptor.getPartitioner(), |
There was a problem hiding this comment.
DatabaseDescriptor.getPartitioner() is called for every iteration, extract it into a variable.
There was a problem hiding this comment.
Extracted partitioner into a local variable before the stream loop.
| if (entry.getKey() != ApplicationState.TOKENS) | ||
| return entry.getKey() + "=" + entry.getValue(); | ||
|
|
||
| VersionedValue value = entry.getValue(); |
There was a problem hiding this comment.
can you make VersionedValue value final?
| int numTokens = TokenSerializer.deserialize(DatabaseDescriptor.getPartitioner(), | ||
| new DataInputStream(new ByteArrayInputStream(value.toBytes()))) | ||
| .size(); | ||
| return entry.getKey() + "=Value(<" + numTokens + " tokens>," + value.version + ')'; |
There was a problem hiding this comment.
numTokens is not used anywhere except once, please inline it.
There was a problem hiding this comment.
I would prefer keeping numTokens as a separate variable here, it clearly documents what deserialize().size() returns, and inlining a multi-line deserialization call directly inside string concatenation gets pretty dense to read.
|
the patch looks good to me on code. but the image is not a representative of this patch as opposed to the image on jira showing the issue. To properly verify the fix, could you please provide a log snippet that includes the TOKENS state, showing it rendered as TOKENS=Value(,X) instead of the raw bytes? |
The previous screenshot did already show it correctly (last line, TOKENS=Value(<16 tokens>,35) at Gossiper.java:2406), just easy to miss in a wide capture. Here's a clearer TOKENS rendered correctly, repeated across multiple lines: Both call sites (2406 and 2079) show TOKENS=Value(<16 tokens>,X) instead of raw bytes across all 5 lines here, from two separate real nodes gossiping with each other.
|
|
@Suhel0328 Aah nice. it even shows TOKENS=Value(<16 tokens>,0) for a node with STATUS_WITH_PORT=BOOT as expected. |
|
+1 from my side. |
| { | ||
| IPartitioner partitioner = DatabaseDescriptor.getPartitioner(); | ||
| return applicationState.entrySet().stream().map(entry -> { | ||
| if (entry.getKey() != ApplicationState.TOKENS) |
There was a problem hiding this comment.
@smiklosovic as ApplicationState.TOKENS is "deprecated since version CEP-21", is there an alternate way?
There was a problem hiding this comment.
imho just leave it like this
| if (entry.getKey() != ApplicationState.TOKENS) | ||
| return entry.getKey() + "=" + entry.getValue(); | ||
|
|
||
| final VersionedValue value = entry.getValue(); |
There was a problem hiding this comment.
I think all of this below is not necessary, we can just say TOKENS=Value(<hidden>) and be done with it, no deserialisation or similar, that can go away, just follow https://issues.apache.org/jira/secure/attachment/12755869/10330.txt
There was a problem hiding this comment.
Updated! Removed the deserialization logic entirely and formatted TOKENS as Value(<hidden>) when present, or not present when missing, aligning directly with CASSANDRA-10330.


toString() on EndpointState was printing the raw TOKENS bytes directly. TOKENS is a serialized token collection stored as an ISO-8859-1 string just to preserve the raw bytes, so printing it as text dumps non-printable control characters into gossip debug logs.
Now TOKENS renders as
Value(<N tokens>,<version>), same shape as every other application state'sValue(<value>,<version>). If it can't be deserialized for any reason, including a corrupt length prefix that could otherwise blow up with an OutOfMemoryError, it falls back toValue(<N undecodable bytes>,<version>)instead of printing raw bytes.Added two tests in EndpointStateTest.java: testToStringDoesNotLeakRawTokenBytes and testToStringHandlesUndecodableTokensValue.
ant test -Dtest.name=EndpointStateTest passes, 4/4.
CASSANDRA-21417