gianm commented on a change in pull request #10730:
URL: https://github.com/apache/druid/pull/10730#discussion_r553572213
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/common/OrderedPartitionableRecord.java
##########
@@ -74,7 +74,7 @@ public SequenceOffsetType getSequenceNumber()
}
@NotNull
- public List<byte[]> getData()
+ public List<? extends ByteEntity> getData()
Review comment:
This change means we're returning a thing that wraps a `ByteBuffer`
instead of a `byte[]`, which opens up potential issues due to callers handling
the position and limit wrong. What are the expectations? Is it ok for callers
to change the position of the underlying buffer or should they refrain? If they
do change it, does that cause problems?
(For example: do we have situations where something calls `getData()` to
parse a value, and that updates the position, and then later on `getData()` is
called again for some other purpose like logging? If so — that'd break as a
result of this change.)
Whatever the case, the javadoc for this method should describe the
conclusion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
