ignite-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anton Vinogradov (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (IGNITE-500) CacheLoadingConcurrentGridStartSelfTest fails
Date Mon, 31 Oct 2016 09:42:58 GMT

    [ https://issues.apache.org/jira/browse/IGNITE-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616309#comment-15616309
] 

Anton Vinogradov edited comment on IGNITE-500 at 10/31/16 9:42 AM:
-------------------------------------------------------------------

Semen, thanks for review and fixes.

1){noformat}as I understand new streaming by 'current topology + affinity' is needed only
when IsolatedUpdater is used (i.e. allowOverwrite=false)?{noformat}
Correct. 

2){noformat}I think you need fix DataStreamerImpl.nodes to use 'cctx.topology().nodes' only
for IsolatedUpdater{noformat}
It's already works this way

{noformat}
if (!allowOverwrite())
            res = cctx.isLocal() ?
                aff.mapKeyToPrimaryAndBackups(cacheName, key, topVer) :
                cctx.topology().nodes(key.partition(), topVer);
{noformat} 

is that what you mention about?

3){noformat}, also you do not need to add new field in DataStreamerRequest, allowOverwriet
is true is updater 'instance of IsolatedUpdater'{noformat}
Updater is an IsolatedUpdater instance from another node.
Possible, in future IsolatedUpdater will be refactored to IsolatedUpdaterV2 or relocated to
another package and "instance of IsolatedUpdater" check at old nodes will fail. 

4){noformat}in DataStreamerImpl constructor you create cache if it does not exists only for
clients. Why this is not needed for server nodes?{noformat}
Fixed & added test.

5){noformat}in Ignite code 'catch (Throwable ex)' is not usually used, why you need catch
Throwable? If this is really needed then you must re-throw Errors.{noformat}
This catchs AssertionErrors as well. Agree that this should be refactored, can you give me
some tips how to do that in proper way?

6){noformat}please add test with server nodes restart in CacheLoadingConcurrentGridStartSelfTest{noformat}
In this case streamer should fail with exception, this checked at another tests, for example
IgniteClientReconnectStreamerTest.

7){noformat}I did not understand changes in GridCompoundFuture, please explain{noformat}
Compound future can fail on exception from another node and it was hard to analyze where Compound
future actually failed, so I extended exception with "current" stack trace. 
Yes, Better solution in to extend stack trace of exceptions gained from another nodes.
Fixed. GridCompoundFuture changes rollbacked.

8){noformat}Also for debug purpose please add topology version in data streamer future (which
is created in GridCacheMvccManager) and print all these futures in GridCachePartitionExchangeManager.dumpPendingObjects.{noformat}
Done.


was (Author: avinogradov):
Semen, thanks for review and fixes.

1){noformat}as I understand new streaming by 'current topology + affinity' is needed only
when IsolatedUpdater is used (i.e. allowOverwrite=false)?{noformat}
Correct. 

2){noformat}I think you need fix DataStreamerImpl.nodes to use 'cctx.topology().nodes' only
for IsolatedUpdater{noformat}
It's already works this way

{noformat}
if (!allowOverwrite())
            res = cctx.isLocal() ?
                aff.mapKeyToPrimaryAndBackups(cacheName, key, topVer) :
                cctx.topology().nodes(key.partition(), topVer);
{noformat} 

is that what you mention about?

3){noformat}, also you do not need to add new field in DataStreamerRequest, allowOverwriet
is true is updater 'instance of IsolatedUpdater'{noformat}
Updater is an IsolatedUpdater instance from another node.
Possible, in future IsolatedUpdater will be refactored to IsolatedUpdaterV2 or relocated to
another package and "instance of IsolatedUpdater" check at old nodes will fail. 

4){noformat}in DataStreamerImpl constructor you create cache if it does not exists only for
clients. Why this is not needed for server nodes?{noformat}
Fixed & added test.

5){noformat}in Ignite code 'catch (Throwable ex)' is not usually used, why you need catch
Throwable? If this is really needed then you must re-throw Errors.{noformat}
This catchs AssertionErrors as well. Agree that this should be refactored, can you give me
some tips how to do that in proper way?

6){noformat}please add test with server nodes restart in CacheLoadingConcurrentGridStartSelfTest{noformat}
In this case streamer should fail with exception, this checked at another tests, for example
IgniteClientReconnectStreamerTest.

7){noformat}I did not understand changes in GridCompoundFuture, please explain{noformat}
Compound future can fail on exception from another node and it was hard to analyze where Compound
future actually failed, so I extended exception with "current" stack trace. 
Yes, Better solution in to extend stack trace of exceptions gained from another nodes.
Fixed.

8){noformat}Also for debug purpose please add topology version in data streamer future (which
is created in GridCacheMvccManager) and print all these futures in GridCachePartitionExchangeManager.dumpPendingObjects.{noformat}
Done.

> CacheLoadingConcurrentGridStartSelfTest fails
> ---------------------------------------------
>
>                 Key: IGNITE-500
>                 URL: https://issues.apache.org/jira/browse/IGNITE-500
>             Project: Ignite
>          Issue Type: Sub-task
>            Reporter: Yakov Zhdanov
>            Assignee: Anton Vinogradov
>            Priority: Critical
>              Labels: Muted_test, important, user-request
>             Fix For: 1.8
>
>         Attachments: ignite-500.log
>
>
> http://apacheignite.readme.io/v1.0/discuss/550865a8e35e9c3b0083af3e



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message