[
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:13 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.с}}. 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)
