abdullah alamoudi has posted comments on this change.

Change subject: Fix Decoding of byte[] Records
......................................................................


Patch Set 2:

(2 comments)

https://asterix-gerrit.ics.uci.edu/#/c/951/2/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/converter/DCPMessageToRecordConverter.java
File 
asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/converter/DCPMessageToRecordConverter.java:

Line 42: public class DCPMessageToRecordConverter implements 
IRecordToRecordWithMetadataAndPKConverter<DCPRequest, char[]> {
> Is the code style correct?
If by Style, you mean the formatting? then I think so.

If by style you mean interfaces and class design, Till and I have been 
discussing going over the interfaces at some point.
For now, this is correct and it means that you give this converter a DCPRequest 
(DCPRequest id a dcp message. I don't know why they named the class DCPRequest 
and I brought it up with them). The char[] means that the returned record 
object has a char[] which can be parsed.


Line 116:             bytes.compact();
> A lot of memory copies here?
the SDK use netty byte buffer which id different from java ByteBuffer. the 
decoder only deals with ByteBuffers and CharBuffer. The parser can only deal 
with char[].
hence, doing all of this data movement.
For now, I think this is okay. At least, I am not creating any objects and all 
the copying is bulk operations which are relatively cheaper.


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/951
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I71c3d8b8dfa5a98123725f139247d2b5ce10012e
Gerrit-PatchSet: 2
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com>
Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to