[GitHub] zhijiangW edited a comment on issue #6705: [FLINK-10356][network] add sanity checks to SpillingAdaptiveSpanningRecordDeserializer

[GitBox <gi...@apache.org>]

zhijiangW edited a comment on issue #6705: [FLINK-10356][network] add sanity checks to SpillingAdaptiveSpanningRecordDeserializer
URL: https://github.com/apache/flink/pull/6705#issuecomment-440208800
 
 
   @NicoK , sorry for the late response! 
   
   I reviewed the whole changes again. To be honest, I still have two concerns in `SpillingAdaptiveSpanningRecordDeserializer`.
   
   1. We add more extra codes for debugging exceptions in main process especially in reading `nonSpanningWrapper`, that would make readable not very well for a little complicated logic, especially for the long `throwDeserializationError` parameters. No matter reading `nonSpanningWrapper` or `spanningWrapper`, if they can internally check whether read more or less as expected, it can make main logics clearly.
   
   2. For reading `spanningWrapper` process, we do not handle the two exceptions together as reading `nonSpanningWrapper`. Left checking less-consumed exception in the process of `moveRemainderToNonSpanningDeserializer`. If we can handle both less-consumed and more-consumed in this process `target.read(this.spanningWrapper.getInputView())`, it may seem better. But I know it seems difficult to do that.
   
   I agree that the added checking information is useful for debugging, and I have not thought of other ways to better solve above concerns, so I can accept the current changes. :) 
   
   Only left one additional comment for duplicate tests.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services