You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by "Sunny Chan, CLSA" <su...@clsa.com> on 2018/05/15 02:17:54 UTC
Null check removal in IGNITE-5779
Hello,
In Ignite-5779 patch, CassandraSessionImpl.java line 289 a null check for row has been removed (before the change: https://github.com/apache/ignite/blob/924b1faa64026107bf933ba441e743cf52cb94d1/modules/cassandra/store/src/main/java/org/apache/ignite/cache/store/cassandra/session/CassandraSessionImpl.java#L289
)
I cannot tell immediately why the null check is no longer needed, especially the row assignment in line 289 can definitely return a null. Is it a mistake or we are expecting BatchExecutionAssistant is able to deal with null?
Thanks.
Sunny Chan
Senior Lead Engineer, Executive Services
D +852 2600 8907 | M +852 6386 1835 | T +852 2600 8888
5/F, One Island East, 18 Westlands Road, Island East, Hong Kong
[:1. Social Media Icons:CLSA_Social Media Icons_linkedin.png]<https://hk.linkedin.com/company/clsa>[:1. Social Media Icons:CLSA_Social Media Icons_twitter.png]<https://twitter.com/clsainsights?lang=en>[:1. Social Media Icons:CLSA_Social Media Icons_youtube.png]<https://www.youtube.com/channel/UC0qWp_lLnOcRYmBlCNQgZKA>[:1. Social Media Icons:CLSA_Social Media Icons_facebook.png]<https://www.facebook.com/clsacommunity/>
clsa.com<https://www.clsa.com/>
Insights. Liquidity. Capital.
[CLSA_RGB]<https://www.clsa.com/member>
A CITIC Securities Company
The content of this communication is intended for the recipient and is subject to CLSA Legal and Regulatory Notices.
These can be viewed at https://www.clsa.com/disclaimer.html or sent to you upon request.
Please consider before printing. CLSA is ISO14001 certified and committed to reducing its impact on the environment.
RE: Null check removal in IGNITE-5779
Posted by "Sunny Chan, CLSA" <su...@clsa.com>.
Actually I have worked it out:
The null check is actually moved to the loadAll method in CassandraCacheStore (https://github.com/apache/ignite/blob/b4a95e7b1574404333912e9cf5564d336fb39130/modules/cassandra/store/src/main/java/org/apache/ignite/cache/store/cassandra/CassandraCacheStore.java#L275). What happens is that the code which keep tracks of the number of items being processed is implemented in the GenericBatchExecutionAssistant (https://github.com/apache/ignite/blob/master/modules/cassandra/store/src/main/java/org/apache/ignite/cache/store/cassandra/session/GenericBatchExecutionAssistant.java) and the null check at CassandraSessionImpl would actually skip the "processed count" logic in GenericBatchExecutionAssistant and you will get into an infinite loop as the processed count has not been updated.
I think it would be useful to update the JavaDoc for BatchExecutionAssistant.process to say that Row can be null (for e.g. when you update an entry, Cassandra returns null for row) - is that a good idea?
Thanks.
-----Original Message-----
From: Dmitry Pavlov [mailto:dpavlov.spb@gmail.com]
Sent: Tuesday, May 15, 2018 9:08 PM
To: dev@ignite.apache.org; Igor Rudyak <ir...@gmail.com>
Cc: igor.rudyak@rallyhealth.com
Subject: Re: Null check removal in IGNITE-5779
Hi Igor,
Could you please help with answering to this question?
Sincerely,
Dmitriy Pavlov
вт, 15 мая 2018 г. в 5:18, Sunny Chan, CLSA <su...@clsa.com>:
> Hello,
>
>
>
> In Ignite-5779 patch, CassandraSessionImpl.java line 289 a null check
> for row has been removed (before the change:
> https://github.com/apache/ignite/blob/924b1faa64026107bf933ba441e743cf
> 52cb94d1/modules/cassandra/store/src/main/java/org/apache/ignite/cache
> /store/cassandra/session/CassandraSessionImpl.java#L289
>
> )
>
>
>
> I cannot tell immediately why the null check is no longer needed,
> especially the row assignment in line 289 can definitely return a
> null. Is it a mistake or we are expecting BatchExecutionAssistant is
> able to deal with null?
>
>
>
> Thanks.
>
>
>
> *Sunny Chan*
>
> *Senior Lead Engineer, Executive Services*
>
> D +852 2600 8907 <+852%202600%208907> | M +852 6386 1835
> <+852%206386%201835> | T +852 2600 8888 <+852%202600%208888>
>
> 5/F, One Island East, 18 Westlands Road
> <https://maps.google.com/?q=18+Westlands+Road&entry=gmail&source=g>,
> Island East, Hong Kong
>
>
>
> [image: :1. Social Media Icons:CLSA_Social Media Icons_linkedin.png]
> <https://hk.linkedin.com/company/clsa>[image: :1. Social Media
> Icons:CLSA_Social Media Icons_twitter.png]
> <https://twitter.com/clsainsights?lang=en>[image: :1. Social Media
> Icons:CLSA_Social Media Icons_youtube.png]
> <https://www.youtube.com/channel/UC0qWp_lLnOcRYmBlCNQgZKA>[image: :1.
> Social Media Icons:CLSA_Social Media Icons_facebook.png]
> <https://www.facebook.com/clsacommunity/>
>
>
>
> *clsa.com* <https://www.clsa.com/>
>
> *Insights. Liquidity. Capital. *
>
>
>
> [image: CLSA_RGB] <https://www.clsa.com/member>
>
>
>
> *A CITIC Securities Company*
>
>
>
> The content of this communication is intended for the recipient and is
> subject to CLSA Legal and Regulatory Notices.
> These can be viewed at https://www.clsa.com/disclaimer.html or sent to
> you upon request.
> Please consider before printing. CLSA is ISO14001 certified and
> committed to reducing its impact on the environment.
>
The content of this communication is intended for the recipient and is subject to CLSA Legal and Regulatory Notices.
These can be viewed at https://www.clsa.com/disclaimer.html or sent to you upon request.
Please consider before printing. CLSA is ISO14001 certified and committed to reducing its impact on the environment.
Re: Null check removal in IGNITE-5779
Posted by Dmitry Pavlov <dp...@gmail.com>.
Hi Igor,
Could you please help with answering to this question?
Sincerely,
Dmitriy Pavlov
вт, 15 мая 2018 г. в 5:18, Sunny Chan, CLSA <su...@clsa.com>:
> Hello,
>
>
>
> In Ignite-5779 patch, CassandraSessionImpl.java line 289 a null check for row has been removed (before the change: https://github.com/apache/ignite/blob/924b1faa64026107bf933ba441e743cf52cb94d1/modules/cassandra/store/src/main/java/org/apache/ignite/cache/store/cassandra/session/CassandraSessionImpl.java#L289
>
> )
>
>
>
> I cannot tell immediately why the null check is no longer needed,
> especially the row assignment in line 289 can definitely return a null. Is
> it a mistake or we are expecting BatchExecutionAssistant is able to deal
> with null?
>
>
>
> Thanks.
>
>
>
> *Sunny Chan*
>
> *Senior Lead Engineer, Executive Services*
>
> D +852 2600 8907 <+852%202600%208907> | M +852 6386 1835
> <+852%206386%201835> | T +852 2600 8888 <+852%202600%208888>
>
> 5/F, One Island East, 18 Westlands Road
> <https://maps.google.com/?q=18+Westlands+Road&entry=gmail&source=g>,
> Island East, Hong Kong
>
>
>
> [image: :1. Social Media Icons:CLSA_Social Media Icons_linkedin.png]
> <https://hk.linkedin.com/company/clsa>[image: :1. Social Media
> Icons:CLSA_Social Media Icons_twitter.png]
> <https://twitter.com/clsainsights?lang=en>[image: :1. Social Media
> Icons:CLSA_Social Media Icons_youtube.png]
> <https://www.youtube.com/channel/UC0qWp_lLnOcRYmBlCNQgZKA>[image: :1.
> Social Media Icons:CLSA_Social Media Icons_facebook.png]
> <https://www.facebook.com/clsacommunity/>
>
>
>
> *clsa.com* <https://www.clsa.com/>
>
> *Insights. Liquidity. Capital. *
>
>
>
> [image: CLSA_RGB] <https://www.clsa.com/member>
>
>
>
> *A CITIC Securities Company*
>
>
>
> The content of this communication is intended for the recipient and is
> subject to CLSA Legal and Regulatory Notices.
> These can be viewed at https://www.clsa.com/disclaimer.html or sent to
> you upon request.
> Please consider before printing. CLSA is ISO14001 certified and committed
> to reducing its impact on the environment.
>