blerer commented on a change in pull request #1251:
URL: https://github.com/apache/cassandra/pull/1251#discussion_r728951552
##########
File path: test/unit/org/apache/cassandra/db/SchemaCQLHelperTest.java
##########
@@ -476,6 +476,6 @@ public void testBooleanCompositeKey() throws Throwable
cfs.getSSTablesForKey("false:true");
execute("insert into %s (t_id, id, ck, nk) VALUES (true, true, false,
true)");
- assertRows(execute("select t_id, id, ck, nk from %s"), row(true,
false, false, true), row(true, true, false, true));
+ assertRows(execute("select t_id, id, ck, nk from %s"), row(true, true,
false, true), row(true, false, false, true));
Review comment:
Coud you explain this change? The ordering should have been maintained.
##########
File path: src/java/org/apache/cassandra/utils/memory/MemoryUtil.java
##########
@@ -216,7 +217,7 @@ public static void setAttachment(ByteBuffer instance,
Object next)
public static ByteBuffer duplicateDirectByteBuffer(ByteBuffer source,
ByteBuffer hollowBuffer)
{
assert source.getClass() == DIRECT_BYTE_BUFFER_CLASS ||
source.getClass() == RO_DIRECT_BYTE_BUFFER_CLASS;
- unsafe.putLong(hollowBuffer, DIRECT_BYTE_BUFFER_ADDRESS_OFFSET,
unsafe.getLong(source, DIRECT_BYTE_BUFFER_ADDRESS_OFFSET));
+ setAddress(hollowBuffer, unsafe.getLong(source,
DIRECT_BYTE_BUFFER_ADDRESS_OFFSET));
Review comment:
On one side the assertion allow to have a read-only direct buffer and on
the otherside it will then be rejected by the `setAddress` method.
Looking at the code it seems that preventing a copy of a read only buffer
might break the code if an `UnbufferedDataOutputStreamPlus` is used to write
some data that come from read-only buffers
##########
File path: src/java/org/apache/cassandra/utils/memory/MemoryUtil.java
##########
@@ -243,6 +244,14 @@ public static void setByteBufferCapacity(ByteBuffer
instance, int capacity)
unsafe.putInt(instance, DIRECT_BYTE_BUFFER_CAPACITY_OFFSET, capacity);
}
+ private static void setAddress(ByteBuffer instance, long address)
+ {
+ if (instance.isReadOnly())
+ throw new ReadOnlyBufferException();
+
+ unsafe.putLong(instance, DIRECT_BYTE_BUFFER_ADDRESS_OFFSET, address);
+ }
+
Review comment:
It seems to me that the check should be done within the methods and not
in a `setAddress` method.
##########
File path: src/java/org/apache/cassandra/db/marshal/AbstractCompositeType.java
##########
@@ -244,7 +244,7 @@ else if (part.equals("_"))
{
comparators.get(i).serializeComparator(bb);
ByteBufferUtil.writeShortLength(bb, component.remaining());
- bb.put(component); // it's ok to consume component as we won't use
it anymore
+ bb.put(component.duplicate()); // it's not ok to consume component
as we did not cdreate it (CASSANDRA-14752)
Review comment:
typo -> cdreate
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
