ignite-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Muzafarov (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (IGNITE-8957) testFailGetLock() constantly fails. Last entry checkpoint history can be empty
Date Sat, 14 Jul 2018 13:06:00 GMT

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

Maxim Muzafarov edited comment on IGNITE-8957 at 7/14/18 1:05 PM:
------------------------------------------------------------------

*Folks,*

I still don't understand you.
 # Why we are merging PR when a lot of questions remains? You are answering the questions
after merge is it right from your point? Is it friendly for community members?
 # As you may know we have [Review Checklist|https://cwiki.apache.org/confluence/display/IGNITE/IEP-4+Baseline+topology+for+caches].
Please, look at {{3.b}}. You are fixing WAL component which is used in whole presistece. I
don't see any TC runs on your PR – [Run :: All - pull/4334/head|https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_RunAll&branch_IgniteTests24Java8=pull%2F4334%2Fhead&tab=buildTypeStatusDiv].
Is it corret?

Current issue is the result of hastily merge of IGNITE-8737. And here we are hurry again,
why? You may heared that whole community are working on [#MakeTeamCityGreenAgain|https://cwiki.apache.org/confluence/display/IGNITE/Make+Teamcity+Green+Again]
and whole progrees of this activity can be eliminatad by inaccurately code changes. I'm a
bit upset here.

I whish Ignite to be an example for other projects.

*About code.* 
My vision is around like this:
 # We should create reusable method for getting index
{code:java}
    /**
     * @param cpEntry Historical entry to evaluate.
     * @return An {@link FileWALPointer#index()} or {@code -1} if there is no checkpoint information.
     */
    private long getCheckpointMarkIdx(@Nullable Map.Entry<Long, CheckpointEntry> cpEntry)
{
        if (cpEntry == null)
            return -1;

        final WALPointer wal = cpEntry.getValue().checkpointMark();

        return wal instanceof FileWALPointer ? ((FileWALPointer)wal).index() : -1;
    }
{code}

 # Reuse it for calculation WAL covered segments
{code:java}
    /**
     * Logs and clears checkpoint history after checkpoint finish.
     *
     * @return List of checkpoints removed from history.
     */
    public List<CheckpointEntry> onCheckpointFinished(GridCacheDatabaseSharedManager.Checkpoint
chp, boolean truncateWal) {

        chp.walSegmentsCovered(
            getCheckpointMarkIdx(
                Optional.ofNullable(histMap.lastEntry())
                    .map(e -> histMap.lowerEntry(e.getKey()))
                    .orElse(null)
            ),
            getCheckpointMarkIdx(histMap.lastEntry())
        );
    ...
}
{code}

[~sergey-chugunov], as you can see code became cleaner and shorter.


was (Author: mmuzaf):
*Folks,*

I still don't understand you.
 # Why we are merging PR when a lot of questions remains? You are answering the questions
after merge is it right from your point? Is it friendly for community members?
 # As you may know we have [Review Checklist|https://cwiki.apache.org/confluence/display/IGNITE/IEP-4+Baseline+topology+for+caches].
Please, look at {{3.b}}. You are fixing WAL component which is used in whole presistece. I
don't see any TC runs on your PR – [Run :: All - pull/4334/head|https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_RunAll&branch_IgniteTests24Java8=pull%2F4334%2Fhead&tab=buildTypeStatusDiv].
Is it corret?

Current issue is the result of hastily merge of IGNITE-8737. And here we are hurry again,
why? You may heared that whole community are working on [#MakeTeamCityGreenAgain|https://cwiki.apache.org/confluence/display/IGNITE/Make+Teamcity+Green+Again]
and whole progrees of this activity can be eliminatad by inaccurately code changes. I'm a
bit upset here.

I whish Ignite to be an example for other projects.

*About code.* 
My vision is around like this:
 # We should create reusable method for getting index
{code:java}
    /**
     * @param cpEntry Historical entry to evaluate.
     * @return An {@link FileWALPointer#index()} or {@code -1} if there is no checkpoint information.
     */
    private long getCheckpointMarkIdx(@Nullable Map.Entry<Long, CheckpointEntry> cpEntry)
{
        if (cpEntry == null)
            return -1;

        final WALPointer wal = cpEntry.getValue().checkpointMark();

        return wal instanceof FileWALPointer ? ((FileWALPointer)wal).index() : -1;
    }
{code}

 # Reuse it for calculation WAL covered segments
{code:java}
    /**
     * Logs and clears checkpoint history after checkpoint finish.
     *
     * @return List of checkpoints removed from history.
     */
    public List<CheckpointEntry> onCheckpointFinished(GridCacheDatabaseSharedManager.Checkpoint
chp, boolean truncateWal) {

        chp.walSegmentsCovered(
            getCheckpointMarkIdx(
                Optional.ofNullable(histMap.lastEntry())
                    .map(e -> histMap.lowerEntry(e.getKey()))
                    .orElse(null)
            ),
            getCheckpointMarkIdx(histMap.lastEntry())
        );
{code}

[~sergey-chugunov], as you can see code became cleaner and shorter.

> testFailGetLock() constantly fails. Last entry checkpoint history can be empty
> ------------------------------------------------------------------------------
>
>                 Key: IGNITE-8957
>                 URL: https://issues.apache.org/jira/browse/IGNITE-8957
>             Project: Ignite
>          Issue Type: Bug
>          Components: persistence
>    Affects Versions: 2.7
>            Reporter: Maxim Muzafarov
>            Assignee: Andrew Medvedev
>            Priority: Major
>              Labels: MakeTeamcityGreenAgain
>             Fix For: 2.7
>
>
> IgniteChangeGlobalStateTest#testFailGetLock constantly fails with exception:
> {code}
> java.lang.AssertionError
> 	at org.apache.ignite.internal.processors.cache.persistence.checkpoint.CheckpointHistory.onCheckpointFinished(CheckpointHistory.java:205)
> 	at org.apache.ignite.internal.processors.cache.persistence.GridCacheDatabaseSharedManager$Checkpointer.markCheckpointEnd(GridCacheDatabaseSharedManager.java:3654)
> 	at org.apache.ignite.internal.processors.cache.persistence.GridCacheDatabaseSharedManager$Checkpointer.doCheckpoint(GridCacheDatabaseSharedManager.java:3178)
> 	at org.apache.ignite.internal.processors.cache.persistence.GridCacheDatabaseSharedManager$Checkpointer.body(GridCacheDatabaseSharedManager.java:2953)
> 	at org.apache.ignite.internal.util.worker.GridWorker.run(GridWorker.java:110)
> 	at java.lang.Thread.run(Thread.java:748)
> {code}
> As Sergey Chugunov [mentioned|https://issues.apache.org/jira/browse/IGNITE-8737?focusedCommentId=16535062&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16535062],
issue can be solved different ways:
> {quote}
> It seems we missed a case when lastEntry may be empty. We may choose here from two options:
> * Check if histMap is empty inside onCheckpointFinished. If it is just don't log anything
(it was the very first checkpoint).
> * Check in caller that there is no history, calculate necessary index in caller and pass
it to onCheckpointFinished to prepare correct log message.{quote}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message