[jira] [Commented] (IGNITE-7831) Throw Exceptions instead of AssertionErrors when reading from corrupted persistence
[ https://issues.apache.org/jira/browse/IGNITE-7831?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16440425#comment-16440425 ] ASF GitHub Bot commented on IGNITE-7831: Github user alex-plekhanov closed the pull request at: https://github.com/apache/ignite/pull/3630 > Throw Exceptions instead of AssertionErrors when reading from corrupted > persistence > --- > > Key: IGNITE-7831 > URL: https://issues.apache.org/jira/browse/IGNITE-7831 > Project: Ignite > Issue Type: Improvement >Reporter: Ilya Lantukh >Assignee: Aleksey Plekhanov >Priority: Major > Labels: iep-14 > Fix For: 2.5 > > > There are a few places in our code where we explicitly throw AssertionErrors > due to inability to correctly read data from persistence and many more places > where we make assertions based on read values. > Assertions are used to indicate problems in internal logic, while persistence > might also get corrupted by various external reasons. It also makes uniform > handling of such issues considerably harder, because exception handling logic > in Ignite ignores Errors. If we want to improve stability and minimize > consequenses of pesistence corruption, we should replace all those > AssertionErrors and asserts with Exceptions, so that current exception > handling mechanisms could be reduce. In a number of situations it means that > instead of causing cluster-wide hang-up problematic node will be > automatically killed. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (IGNITE-7831) Throw Exceptions instead of AssertionErrors when reading from corrupted persistence
[ https://issues.apache.org/jira/browse/IGNITE-7831?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16423741#comment-16423741 ] Dmitriy Pavlov commented on IGNITE-7831: [~ilantukh], [~alex_pl], thank you for this contribution. I was going to create exactly the same issue, but it is already fixed. > Throw Exceptions instead of AssertionErrors when reading from corrupted > persistence > --- > > Key: IGNITE-7831 > URL: https://issues.apache.org/jira/browse/IGNITE-7831 > Project: Ignite > Issue Type: Improvement >Reporter: Ilya Lantukh >Assignee: Aleksey Plekhanov >Priority: Major > Labels: iep-14 > Fix For: 2.5 > > > There are a few places in our code where we explicitly throw AssertionErrors > due to inability to correctly read data from persistence and many more places > where we make assertions based on read values. > Assertions are used to indicate problems in internal logic, while persistence > might also get corrupted by various external reasons. It also makes uniform > handling of such issues considerably harder, because exception handling logic > in Ignite ignores Errors. If we want to improve stability and minimize > consequenses of pesistence corruption, we should replace all those > AssertionErrors and asserts with Exceptions, so that current exception > handling mechanisms could be reduce. In a number of situations it means that > instead of causing cluster-wide hang-up problematic node will be > automatically killed. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (IGNITE-7831) Throw Exceptions instead of AssertionErrors when reading from corrupted persistence
[ https://issues.apache.org/jira/browse/IGNITE-7831?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16422857#comment-16422857 ] Andrey Gura commented on IGNITE-7831: - [~alex_pl] LGTM! Merged into master branch. Thanks for contribution! > Throw Exceptions instead of AssertionErrors when reading from corrupted > persistence > --- > > Key: IGNITE-7831 > URL: https://issues.apache.org/jira/browse/IGNITE-7831 > Project: Ignite > Issue Type: Improvement >Reporter: Ilya Lantukh >Assignee: Aleksey Plekhanov >Priority: Major > Labels: iep-14 > Fix For: 2.5 > > > There are a few places in our code where we explicitly throw AssertionErrors > due to inability to correctly read data from persistence and many more places > where we make assertions based on read values. > Assertions are used to indicate problems in internal logic, while persistence > might also get corrupted by various external reasons. It also makes uniform > handling of such issues considerably harder, because exception handling logic > in Ignite ignores Errors. If we want to improve stability and minimize > consequenses of pesistence corruption, we should replace all those > AssertionErrors and asserts with Exceptions, so that current exception > handling mechanisms could be reduce. In a number of situations it means that > instead of causing cluster-wide hang-up problematic node will be > automatically killed. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (IGNITE-7831) Throw Exceptions instead of AssertionErrors when reading from corrupted persistence
[ https://issues.apache.org/jira/browse/IGNITE-7831?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16398327#comment-16398327 ] ASF GitHub Bot commented on IGNITE-7831: GitHub user alex-plekhanov opened a pull request: https://github.com/apache/ignite/pull/3630 IGNITE-7831 Throw Exceptions instead of AssertionErrors when reading from corrupted persistence You can merge this pull request into a Git repository by running: $ git pull https://github.com/alex-plekhanov/ignite ignite-7831 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/ignite/pull/3630.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3630 commit 1698c15ce0d47d5fbb8dbc633fbf007fb5e7c0e3 Author: Aleksey Plekhanov Date: 2018-03-13T11:09:54Z IGNITE-7831 Throw Exceptions instead of AssertionErrors when reading from corrupted persistence commit 905a1a0065eb853f70e5bb86bb850c27707f4b8d Author: Aleksey Plekhanov Date: 2018-03-13T11:46:10Z IGNITE-7831 WIP commit fd94878610d70697181e6ffcfe939073d8bc273f Author: Aleksey Plekhanov Date: 2018-03-14T08:39:49Z IGNITE-7831 WIP > Throw Exceptions instead of AssertionErrors when reading from corrupted > persistence > --- > > Key: IGNITE-7831 > URL: https://issues.apache.org/jira/browse/IGNITE-7831 > Project: Ignite > Issue Type: Improvement >Reporter: Ilya Lantukh >Assignee: Aleksey Plekhanov >Priority: Major > Labels: iep-14 > Fix For: 2.5 > > > There are a few places in our code where we explicitly throw AssertionErrors > due to inability to correctly read data from persistence and many more places > where we make assertions based on read values. > Assertions are used to indicate problems in internal logic, while persistence > might also get corrupted by various external reasons. It also makes uniform > handling of such issues considerably harder, because exception handling logic > in Ignite ignores Errors. If we want to improve stability and minimize > consequenses of pesistence corruption, we should replace all those > AssertionErrors and asserts with Exceptions, so that current exception > handling mechanisms could be reduce. In a number of situations it means that > instead of causing cluster-wide hang-up problematic node will be > automatically killed. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (IGNITE-7831) Throw Exceptions instead of AssertionErrors when reading from corrupted persistence
[
https://issues.apache.org/jira/browse/IGNITE-7831?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16386105#comment-16386105
]
Ilya Lantukh commented on IGNITE-7831:
--
[~alex_pl], problems that I encountered and that I'd like to see fixed and
removed include those you discovered. Making assertions based on ability to
create directory and throwing explicit AssertionErrors when WAL doesn't have
enough data to restore page are things that should be fixed.
One of real cases that I encountered quite recently is method
GridCacheOffheapManager.GridCacheDataStore.getOrAllocatePartitionMetas():
{noformat}
else {
PagePartitionMetaIO io = PageIO.getPageIO(pageAddr);
treeRoot = io.getTreeRoot(pageAddr);
reuseListRoot = io.getReuseListRoot(pageAddr);
assert PageIdUtils.flag(treeRoot) == PageMemory.FLAG_DATA :
U.hexLong(treeRoot) + ", part=" + partId + ", grpId=" + grpId;
assert PageIdUtils.flag(reuseListRoot) == PageMemory.FLAG_DATA :
U.hexLong(reuseListRoot) + ", part=" + partId + ", grpId=" + grpId;
}
{noformat}
Ideally, I'd like to ensure that all assertions for reading data from memory
pages are properly handled and lead to node invalidation
(NodeInvalidator.invalidate(...)). Replacing those "assert"s with "throw"s
might be inconvenient and make code less readable, but making sure that they
are caught, processed and re-thrown will be beneficial.
> Throw Exceptions instead of AssertionErrors when reading from corrupted
> persistence
> ---
>
> Key: IGNITE-7831
> URL: https://issues.apache.org/jira/browse/IGNITE-7831
> Project: Ignite
> Issue Type: Improvement
>Reporter: Ilya Lantukh
>Assignee: Aleksey Plekhanov
>Priority: Major
> Labels: iep-14
> Fix For: 2.5
>
>
> There are a few places in our code where we explicitly throw AssertionErrors
> due to inability to correctly read data from persistence and many more places
> where we make assertions based on read values.
> Assertions are used to indicate problems in internal logic, while persistence
> might also get corrupted by various external reasons. It also makes uniform
> handling of such issues considerably harder, because exception handling logic
> in Ignite ignores Errors. If we want to improve stability and minimize
> consequenses of pesistence corruption, we should replace all those
> AssertionErrors and asserts with Exceptions, so that current exception
> handling mechanisms could be reduce. In a number of situations it means that
> instead of causing cluster-wide hang-up problematic node will be
> automatically killed.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (IGNITE-7831) Throw Exceptions instead of AssertionErrors when reading from corrupted persistence
[
https://issues.apache.org/jira/browse/IGNITE-7831?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16383714#comment-16383714
]
Aleksey Plekhanov commented on IGNITE-7831:
---
[~ilantukh], I search for assertions in the persistance implementation, there
are about 7 hundreds occurrencies, but almost all of this assertions are used
to ensure internal logic and not depend on external reasons.
Also, most of explicitly thrown AssertionErrors in the persistance
implementation are rethrown assertions with additional information.
I found only couple of cases which can potentialy match your description:
{{PageMemoryImpl#tryToRestorePage:}}
{code:java}
throw new AssertionError(String.format(
"Page is broken. Can't restore it from WAL. (grpId = %d, pageId = %X).",
fullId.groupId(), fullId.pageId()
));
{code}
{{GridTreePrinter#getChildren}}:
{code:java}
catch (IgniteCheckedException ignored) {
throw new AssertionError("Can not acquire page.");
}
{code}
But this method used only for dumping debug information in the test framework.
{{FilePageStoreManager#beforeCacheGroupStart}}:
{code:java}
assert dir.exists();
{code}
{{FilePageStoreManager#storeCacheData}}:
{code:java}
assert cacheWorkDir.exists() : "Work directory does not exist: " + cacheWorkDir;
{code}
But this method creates dir (and checks result code) before assertion.
Did you mean these cases or something else? If something else, do you have
stacktraces or any description of real cases?
> Throw Exceptions instead of AssertionErrors when reading from corrupted
> persistence
> ---
>
> Key: IGNITE-7831
> URL: https://issues.apache.org/jira/browse/IGNITE-7831
> Project: Ignite
> Issue Type: Improvement
>Reporter: Ilya Lantukh
>Assignee: Aleksey Plekhanov
>Priority: Major
> Labels: iep-14
> Fix For: 2.5
>
>
> There are a few places in our code where we explicitly throw AssertionErrors
> due to inability to correctly read data from persistence and many more places
> where we make assertions based on read values.
> Assertions are used to indicate problems in internal logic, while persistence
> might also get corrupted by various external reasons. It also makes uniform
> handling of such issues considerably harder, because exception handling logic
> in Ignite ignores Errors. If we want to improve stability and minimize
> consequenses of pesistence corruption, we should replace all those
> AssertionErrors and asserts with Exceptions, so that current exception
> handling mechanisms could be reduce. In a number of situations it means that
> instead of causing cluster-wide hang-up problematic node will be
> automatically killed.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (IGNITE-7831) Throw Exceptions instead of AssertionErrors when reading from corrupted persistence
[ https://issues.apache.org/jira/browse/IGNITE-7831?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16380431#comment-16380431 ] Andrey Gura commented on IGNITE-7831: - [~ilantukh], could you please give more details about the issue? What cases do you mean exactly? Are there any proposed solutions? Etc. > Throw Exceptions instead of AssertionErrors when reading from corrupted > persistence > --- > > Key: IGNITE-7831 > URL: https://issues.apache.org/jira/browse/IGNITE-7831 > Project: Ignite > Issue Type: Improvement >Reporter: Ilya Lantukh >Priority: Major > Labels: iep-14 > > Assertions are used to indicate problems in internal logic, while persistence > might also get corrupted by various external reasons. It also makes uniform > handling of such issues considerably harder, because most places of code in > Ignite ignore Errors. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (IGNITE-7831) Throw Exceptions instead of AssertionErrors when reading from corrupted persistence
[ https://issues.apache.org/jira/browse/IGNITE-7831?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16378710#comment-16378710 ] Ilya Lantukh commented on IGNITE-7831: -- Assertions are used to indicate problems in internal logic, while persistence might also get corrupted by various external reasons. It also makes uniform handling of such issues considerably harder, because most places of code in Ignite ignore Errors. > Throw Exceptions instead of AssertionErrors when reading from corrupted > persistence > --- > > Key: IGNITE-7831 > URL: https://issues.apache.org/jira/browse/IGNITE-7831 > Project: Ignite > Issue Type: Improvement >Reporter: Ilya Lantukh >Priority: Major > Labels: iep-14 > -- This message was sent by Atlassian JIRA (v7.6.3#76005)
