[
https://issues.apache.org/jira/browse/CASSANDRA-21575?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
sepuri sai krishna updated CASSANDRA-21575:
-------------------------------------------
Since Version: 4.0
Test and Documentation Plan:
Unit tests only; no documentation change is needed as this is not a user-facing
or
configuration change.
* ForwardingInfoTest.testLargeMessageIdsRoundTrip - round-trips ids across
the whole
unsigned 32-bit range, including Integer.MAX_VALUE + 1 and 0xFFFFFFFF, for
every
supported messaging version, asserting serializedSize matches the bytes
written.
Verified to fail without the ForwardingInfo change:
"VIntOutOfRangeException: 2147483648 is out of range for a 32-bit integer".
* MessageTest.testIdsRemainUnsignedAcrossCounterWrap - ids stay non-negative,
stay within
32 unsigned bits, and encode within 5 bytes across the counter wrap
boundary.
* MessageTest.testLargeIdRoundTrips - message headers carrying ids from the
upper half of
the range round-trip for every supported messaging version.
Green locally: ant jar, ant checkstyle checkstyle-test (2907 files), and
MessageTest (11),
ForwardingInfoTest (2), MessageSerializationPropertyTest (2),
MessageDeliveryTest (4).
Description:
{{Message.nextId()}} draws from a shared {{AtomicInteger}} and widens the
result to a {{long}}:
{code:java}
private static final AtomicInteger nextId = new AtomicInteger(0);
private static long nextId()
{
long id;
do
{
id = nextId.incrementAndGet();
}
while (id == NO_ID);
return id;
}
{code}
Once the counter passes {{Integer.MAX_VALUE}} it wraps to
{{Integer.MIN_VALUE}}, and the negative
{{int}} sign-extends into a negative {{long}}. {{Message.Serializer}} writes
the id as an unsigned
vint:
{code:java}
out.writeUnsignedVInt(header.id);
{code}
A negative long has all its high bits set, so it always encodes at the maximum
width of 9 bytes:
|| id || wire bytes ||
| 1 | 1 |
| 2147483647 | 5 |
| -2147483648 (first wrap) | 9 |
| -1 | 9 |
That is an extra 4 bytes on the header of every internode message, sustained
for the 2^31 messages
until the counter cycles back through the positive range, and then again on
every subsequent wrap.
A node sending 50k messages/sec reaches the first wrap in roughly 12 hours, so
this is steady state
for any long-running cluster rather than an edge case.
Nothing depends on the sign of an id. They are opaque correlation keys, used
only for matching
responses to callbacks in {{RequestCallbacks}}, so keeping them non-negative is
behaviour preserving.
The wire format is unchanged; only the range of values written changes.
h3. Coupled defect in ForwardingInfo
{{ForwardingInfo}} is the {{FORWARD_TO}} parameter used for inter-DC write
forwarding. It writes ids
with the 64-bit form and sizes them the same way, but reads them back with the
32-bit form:
{code:java}
// serialize
out.writeUnsignedVInt(ids[i]);
// serializedSize
size += computeUnsignedVIntSize(ids[i]);
// deserialize
ids[i] = in.readUnsignedVInt32(); // mismatch
{code}
This is the only place in the codebase that reads a message id at 32 bits;
every other reader uses
{{readUnsignedVInt}}/{{getUnsignedVInt}}. It is currently harmless only by
accident: ids always fit in
{{int}} today, because they come from an {{int}} counter, so the
{{checkedCast}} inside
{{readUnsignedVInt32}} happens to succeed even for the sign-extended negative
values.
That makes it a blocker for the fix rather than a separate cleanup. Once ids
span the full unsigned
32-bit range, this read throws:
{noformat}
fixed nextId, post-wrap id=2147483648 ForwardingInfo.deserialize:
VIntOutOfRangeException
fixed nextId, max id=4294967295 ForwardingInfo.deserialize:
VIntOutOfRangeException
{noformat}
which would break inter-DC write forwarding. Both changes therefore belong in
the same commit.
The same narrowing is present on 4.0 and 4.1, spelled differently:
{code:java}
ids[i] = version >= VERSION_40 ? Ints.checkedCast(in.readUnsignedVInt()) :
in.readInt();
{code}
so both parts of this ticket apply from 4.0 onwards. Before 4.0 the id was
written as a fixed-width
int rather than a vint, so the inflation does not arise there. The patch is
against trunk; happy to
prepare backports for whichever branches the reviewer wants this on.
h3. Fix
Mask the counter so ids stay in {{[0, 2^32)}}. This covers the same number of
distinct values, keeps
them non-negative, and bounds the encoding at 5 bytes. Change
{{ForwardingInfo.deserialize}} to use
{{readUnsignedVInt}}, matching its own serialize and serializedSize.
h3. Tests
The existing {{ForwardingInfoTest}} only ever used ids 44-49, which is why the
width mismatch was
never exercised. Added:
* {{ForwardingInfoTest.testLargeMessageIdsRoundTrip}} - round-trips ids across
the whole unsigned
32-bit range, including {{Integer.MAX_VALUE + 1}} and {{0xFFFFFFFF}}, asserting
{{serializedSize}}
matches the bytes written. Fails with {{VIntOutOfRangeException: 2147483648 is
out of range for a
32-bit integer}} without the {{ForwardingInfo}} fix.
* {{MessageTest.testIdsRemainUnsignedAcrossCounterWrap}} - asserts ids stay
non-negative, stay within
32 unsigned bits, and encode within 5 bytes across the wrap boundary.
* {{MessageTest.testLargeIdRoundTrips}} - round-trips a message header carrying
ids from the upper
half of the range, for every supported messaging version.
was:
{{Message.nextId()}} draws from a shared {{AtomicInteger}} and widens the
result to a {{long}}:
{code:java}
private static final AtomicInteger nextId = new AtomicInteger(0);
private static long nextId()
{
long id;
do
{
id = nextId.incrementAndGet();
}
while (id == NO_ID);
return id;
}
{code}
Once the counter passes {{Integer.MAX_VALUE}} it wraps to
{{Integer.MIN_VALUE}}, and the negative
{{int}} sign-extends into a negative {{long}}. {{Message.Serializer}} writes
the id as an unsigned
vint:
{code:java}
out.writeUnsignedVInt(header.id);
{code}
A negative long has all its high bits set, so it always encodes at the maximum
width of 9 bytes:
|| id || wire bytes ||
| 1 | 1 |
| 2147483647 | 5 |
| -2147483648 (first wrap) | 9 |
| -1 | 9 |
That is an extra 4 bytes on the header of every internode message, sustained
for the 2^31 messages
until the counter cycles back through the positive range, and then again on
every subsequent wrap.
A node sending 50k messages/sec reaches the first wrap in roughly 12 hours, so
this is steady state
for any long-running cluster rather than an edge case.
Nothing depends on the sign of an id. They are opaque correlation keys, used
only for matching
responses to callbacks in {{RequestCallbacks}}, so keeping them non-negative is
behaviour preserving.
The wire format is unchanged; only the range of values written changes.
h3. Coupled defect in ForwardingInfo
{{ForwardingInfo}} is the {{FORWARD_TO}} parameter used for inter-DC write
forwarding. It writes ids
with the 64-bit form and sizes them the same way, but reads them back with the
32-bit form:
{code:java}
// serialize
out.writeUnsignedVInt(ids[i]);
// serializedSize
size += computeUnsignedVIntSize(ids[i]);
// deserialize
ids[i] = in.readUnsignedVInt32(); // mismatch
{code}
This is the only place in the codebase that reads a message id at 32 bits;
every other reader uses
{{readUnsignedVInt}}/{{getUnsignedVInt}}. It is currently harmless only by
accident: ids always fit in
{{int}} today, because they come from an {{int}} counter, so the
{{checkedCast}} inside
{{readUnsignedVInt32}} happens to succeed even for the sign-extended negative
values.
The same narrowing is present on 4.0 and 4.1, spelled differently:
{code:java}
ids[i] = version >= VERSION_40 ? Ints.checkedCast(in.readUnsignedVInt()) :
in.readInt();
{code}
so both parts of this ticket apply from 4.0 onwards. The attached patch is
against trunk; happy to
prepare backports for whichever branches the reviewer wants this on.
That makes it a blocker for the fix rather than a separate cleanup. Once ids
span the full unsigned
32-bit range, this read throws:
{noformat}
fixed nextId, post-wrap id=2147483648 ForwardingInfo.deserialize:
VIntOutOfRangeException
fixed nextId, max id=4294967295 ForwardingInfo.deserialize:
VIntOutOfRangeException
{noformat}
which would break inter-DC write forwarding. Both changes therefore belong in
the same commit.
h3. Fix
Mask the counter so ids stay in {{[0, 2^32)}}. This covers the same number of
distinct values, keeps
them non-negative, and bounds the encoding at 5 bytes. Change
{{ForwardingInfo.deserialize}} to use
{{readUnsignedVInt}}, matching its own serialize and serializedSize.
h3. Tests
The existing {{ForwardingInfoTest}} only ever used ids 44-49, which is why the
width mismatch was
never exercised. Added:
* {{ForwardingInfoTest.testLargeMessageIdsRoundTrip}} - round-trips ids across
the whole unsigned
32-bit range, including {{Integer.MAX_VALUE + 1}} and {{0xFFFFFFFF}}, asserting
{{serializedSize}}
matches the bytes written. Fails with {{VIntOutOfRangeException: 2147483648 is
out of range for a
32-bit integer}} without the {{ForwardingInfo}} fix.
* {{MessageTest.testIdsRemainUnsignedAcrossCounterWrap}} - asserts ids stay
non-negative, stay within
32 unsigned bits, and encode within 5 bytes across the wrap boundary.
* {{MessageTest.testLargeIdRoundTrips}} - round-trips a message header carrying
ids from the upper
half of the range, for every supported messaging version.
> Message ids become negative once the id counter wraps, inflating every
> message header
> -------------------------------------------------------------------------------------
>
> Key: CASSANDRA-21575
> URL: https://issues.apache.org/jira/browse/CASSANDRA-21575
> Project: Apache Cassandra
> Issue Type: Bug
> Components: Messaging/Internode
> Reporter: sepuri sai krishna
> Assignee: sepuri sai krishna
> Priority: Normal
>
> {{Message.nextId()}} draws from a shared {{AtomicInteger}} and widens the
> result to a {{long}}:
> {code:java}
> private static final AtomicInteger nextId = new AtomicInteger(0);
> private static long nextId()
> {
> long id;
> do
> {
> id = nextId.incrementAndGet();
> }
> while (id == NO_ID);
> return id;
> }
> {code}
> Once the counter passes {{Integer.MAX_VALUE}} it wraps to
> {{Integer.MIN_VALUE}}, and the negative
> {{int}} sign-extends into a negative {{long}}. {{Message.Serializer}} writes
> the id as an unsigned
> vint:
> {code:java}
> out.writeUnsignedVInt(header.id);
> {code}
> A negative long has all its high bits set, so it always encodes at the
> maximum width of 9 bytes:
> || id || wire bytes ||
> | 1 | 1 |
> | 2147483647 | 5 |
> | -2147483648 (first wrap) | 9 |
> | -1 | 9 |
> That is an extra 4 bytes on the header of every internode message, sustained
> for the 2^31 messages
> until the counter cycles back through the positive range, and then again on
> every subsequent wrap.
> A node sending 50k messages/sec reaches the first wrap in roughly 12 hours,
> so this is steady state
> for any long-running cluster rather than an edge case.
> Nothing depends on the sign of an id. They are opaque correlation keys, used
> only for matching
> responses to callbacks in {{RequestCallbacks}}, so keeping them non-negative
> is behaviour preserving.
> The wire format is unchanged; only the range of values written changes.
> h3. Coupled defect in ForwardingInfo
> {{ForwardingInfo}} is the {{FORWARD_TO}} parameter used for inter-DC write
> forwarding. It writes ids
> with the 64-bit form and sizes them the same way, but reads them back with
> the 32-bit form:
> {code:java}
> // serialize
> out.writeUnsignedVInt(ids[i]);
> // serializedSize
> size += computeUnsignedVIntSize(ids[i]);
> // deserialize
> ids[i] = in.readUnsignedVInt32(); // mismatch
> {code}
> This is the only place in the codebase that reads a message id at 32 bits;
> every other reader uses
> {{readUnsignedVInt}}/{{getUnsignedVInt}}. It is currently harmless only by
> accident: ids always fit in
> {{int}} today, because they come from an {{int}} counter, so the
> {{checkedCast}} inside
> {{readUnsignedVInt32}} happens to succeed even for the sign-extended negative
> values.
> That makes it a blocker for the fix rather than a separate cleanup. Once ids
> span the full unsigned
> 32-bit range, this read throws:
> {noformat}
> fixed nextId, post-wrap id=2147483648 ForwardingInfo.deserialize:
> VIntOutOfRangeException
> fixed nextId, max id=4294967295 ForwardingInfo.deserialize:
> VIntOutOfRangeException
> {noformat}
> which would break inter-DC write forwarding. Both changes therefore belong in
> the same commit.
> The same narrowing is present on 4.0 and 4.1, spelled differently:
> {code:java}
> ids[i] = version >= VERSION_40 ? Ints.checkedCast(in.readUnsignedVInt()) :
> in.readInt();
> {code}
> so both parts of this ticket apply from 4.0 onwards. Before 4.0 the id was
> written as a fixed-width
> int rather than a vint, so the inflation does not arise there. The patch is
> against trunk; happy to
> prepare backports for whichever branches the reviewer wants this on.
> h3. Fix
> Mask the counter so ids stay in {{[0, 2^32)}}. This covers the same number of
> distinct values, keeps
> them non-negative, and bounds the encoding at 5 bytes. Change
> {{ForwardingInfo.deserialize}} to use
> {{readUnsignedVInt}}, matching its own serialize and serializedSize.
> h3. Tests
> The existing {{ForwardingInfoTest}} only ever used ids 44-49, which is why
> the width mismatch was
> never exercised. Added:
> * {{ForwardingInfoTest.testLargeMessageIdsRoundTrip}} - round-trips ids
> across the whole unsigned
> 32-bit range, including {{Integer.MAX_VALUE + 1}} and {{0xFFFFFFFF}},
> asserting {{serializedSize}}
> matches the bytes written. Fails with {{VIntOutOfRangeException: 2147483648
> is out of range for a
> 32-bit integer}} without the {{ForwardingInfo}} fix.
> * {{MessageTest.testIdsRemainUnsignedAcrossCounterWrap}} - asserts ids stay
> non-negative, stay within
> 32 unsigned bits, and encode within 5 bytes across the wrap boundary.
> * {{MessageTest.testLargeIdRoundTrips}} - round-trips a message header
> carrying ids from the upper
> half of the range, for every supported messaging version.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
