Re: Problem with reading incomplete payload - IGNITE-7153

[Dmitriy Pavlov <dp...@apache.org>]

Hi Igniters,

I'm trying to reach the author of the fix because the ticket is still in In
Progress.

Could you please advice me how to handle it (because fix seems to be
useful)? Can we set Patch Available status by lazy consensus and review
possibly incomplete fix?
https://issues.apache.org/jira/browse/IGNITE-7153

Sincerely,
Dmitriy Pavlov

пт, 2 нояб. 2018 г. в 13:20, Michael Fong <mc...@gmail.com>:

> Hi Yakov,
>
> Thanks so much for your analysis.
>
> Parser expects chunks to be complete and has all the data to read entire
> > message, but this is not guaranteed and single message can arrive in
> > several chunks.
>
> This is indeed the the assumption to my implementation. I have not come up
> a another parsing algorithm to handle this rainy day case. Perhaps, it
> would require more refactoring on existing code. In addition, I might need
> to check how Redis dev implements the parser state machine.
>
> I would be interested to see how current implementation (based on
> 2.6/master) behaves if we intentionally split the message into chunks as
> you suggested for the reproducer.
>
> Regards,
>
> Michael
>
> On Wed, Oct 31, 2018 at 7:08 PM Yakov Zhdanov <yz...@apache.org> wrote:
>
> > Hi Mike!
> >
> > Thanks for reproducer. Now I understand the problem. NIO worker reads
> > chunks from the network and notifies the parser on data read. Parser
> > expects chunks to be complete and has all the data to read entire
> message,
> > but this is not guaranteed and single message can arrive in several
> chunks.
> > Which is demostrated by your test.
> >
> > The problem is inside GridRedisProtocolParser. We should add ability to
> > store the parsing context if we do not have all the data to complete
> > message parsing, as it is done, for example in GridBufferedParser. So, it
> > is definitely an issue and should be fixed by adding parsing state. I see
> > you attempted to do so in PR
> > https://github.com/apache/ignite/pull/5044/files. I did not do a formal
> > review, so let's ask community to review your patch.
> >
> > Couple of comments about your reproducer.
> >
> > 1. Let's dump a proper Redis message bytes sent by Jedis.
> > 2. Let's split this dump into 5 chunks and send them with 100 ms delays.
> >
> > This should fail before fix is applied, and should pass with proper
> message
> > parsed after we have the issue fixed.
> >
> > Thanks!
> >
> > --Yakov
> >
>

Re: Problem with reading incomplete payload - IGNITE-7153

[Ilya Kasnacheev <il...@gmail.com>]

Hello!

I don't see why we can't. Just take it over, run tests, move to PA, etc.

Regards,
-- 
Ilya Kasnacheev


чт, 3 янв. 2019 г. в 17:00, Dmitriy Pavlov <dp...@apache.org>:

> Hi Igniters,
>
> I'm trying to reach the author of the fix because the ticket is still in In
> Progress.
>
> Could you please advice me how to handle it (because fix seems to be
> useful)? Can we set Patch Available status by lazy consensus and review
> possibly incomplete fix?
> https://issues.apache.org/jira/browse/IGNITE-7153
>
> Sincerely,
> Dmitriy Pavlov
>
> пт, 2 нояб. 2018 г. в 13:20, Michael Fong <mc...@gmail.com>:
>
> > Hi Yakov,
> >
> > Thanks so much for your analysis.
> >
> > Parser expects chunks to be complete and has all the data to read entire
> > > message, but this is not guaranteed and single message can arrive in
> > > several chunks.
> >
> > This is indeed the the assumption to my implementation. I have not come
> up
> > a another parsing algorithm to handle this rainy day case. Perhaps, it
> > would require more refactoring on existing code. In addition, I might
> need
> > to check how Redis dev implements the parser state machine.
> >
> > I would be interested to see how current implementation (based on
> > 2.6/master) behaves if we intentionally split the message into chunks as
> > you suggested for the reproducer.
> >
> > Regards,
> >
> > Michael
> >
> > On Wed, Oct 31, 2018 at 7:08 PM Yakov Zhdanov <yz...@apache.org>
> wrote:
> >
> > > Hi Mike!
> > >
> > > Thanks for reproducer. Now I understand the problem. NIO worker reads
> > > chunks from the network and notifies the parser on data read. Parser
> > > expects chunks to be complete and has all the data to read entire
> > message,
> > > but this is not guaranteed and single message can arrive in several
> > chunks.
> > > Which is demostrated by your test.
> > >
> > > The problem is inside GridRedisProtocolParser. We should add ability to
> > > store the parsing context if we do not have all the data to complete
> > > message parsing, as it is done, for example in GridBufferedParser. So,
> it
> > > is definitely an issue and should be fixed by adding parsing state. I
> see
> > > you attempted to do so in PR
> > > https://github.com/apache/ignite/pull/5044/files. I did not do a
> formal
> > > review, so let's ask community to review your patch.
> > >
> > > Couple of comments about your reproducer.
> > >
> > > 1. Let's dump a proper Redis message bytes sent by Jedis.
> > > 2. Let's split this dump into 5 chunks and send them with 100 ms
> delays.
> > >
> > > This should fail before fix is applied, and should pass with proper
> > message
> > > parsed after we have the issue fixed.
> > >
> > > Thanks!
> > >
> > > --Yakov
> > >
> >
>