Null check removal in IGNITE-5779

[Sunny Chan, CLSA]

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  [phone]  |  M  [phone]  |  T  [phone]

5/F, One Island East, 18 Westlands Road, Island East, Hong Kong

 

 

clsa.com

Insights. Liquidity. Capital.

 

 

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.

[Dmitriy Pavlov]

Hi Igor,

Could you please help with answering to this question?

Sincerely,
Dmitriy Pavlov

вт, 15 мая 2018 г. в 5:18, Sunny Chan, CLSA <[hidden email]>:

> 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  [phone] <+852%202600%208907>  |  M  [phone]
> <+852%206386%201835>  |  T  [phone] <+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.
>

[Sunny Chan, CLSA]

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:[hidden email]]
Sent: Tuesday, May 15, 2018 9:08 PM
To: [hidden email]; Igor Rudyak <[hidden email]>
Cc: [hidden email]
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 <[hidden email]>:

> 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  [phone] <+852%202600%208907>  |  M  [phone]
> <+852%206386%201835>  |  T  [phone] <+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.