New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FLINK-6284 Incorrect sorting of completed checkpoints in ZooKeeperCompletedCheckpointStore #3881

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ramkrish86
Contributor

ramkrish86 commented May 12, 2017

ZooKeeperCompletedCheckpointStore

Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.

  • General

    • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
    • The pull request addresses only one issue
    • Each commit in the PR has a meaningful commit message (including the JIRA id)
  • Documentation

    • Documentation has been added for new functionality
    • Old documentation affected by the pull request has been updated
    • JavaDoc for public methods has been added
  • Tests & Build

    • Functionality added by the pull request is covered by tests
    • mvn clean verify has been executed successfully locally or a Travis build has passed

Making use of the Zookeeper's getChildren() API directly so that it just creates a list in the sequence order. If we go with the ZKPaths API then we need to do some sorting by converting the List to List.

@tillrohrmann

Thanks for your contribution @ramkrish86. But I think the PR does not fix the underlying problem because it relies on the fact that ZooKeeper#getChildren returns the children in the right order, which it does not guarantee if I'm not mistaken.

@tillrohrmann

Thanks for your contribution @ramkrish86. I still think that this PR does not sufficiently solve the problem. I guess we should either use the czxid for sorting or adapt the path generation to make it sortable.

@tillrohrmann

This comment has been minimized.

Show comment
Hide comment
@tillrohrmann

tillrohrmann May 12, 2017

Contributor

Hi @ramkrish86, I might have found an easy way to solve the problem. Take a look at tillrohrmann@5bd4993. If this solves the problem, then I would open a PR with it.

Contributor

tillrohrmann commented May 12, 2017

Hi @ramkrish86, I might have found an easy way to solve the problem. Take a look at tillrohrmann@5bd4993. If this solves the problem, then I would open a PR with it.

@ramkrish86

This comment has been minimized.

Show comment
Hide comment
@ramkrish86

ramkrish86 May 12, 2017

Contributor

@tillrohrmann
Thanks for the new PR. I just executed your change with 101, 99 , 100 as the checkpoint order. In this case 100 should be the latest one though the actual ids are not sorted. But with your change and my earlier commit it will always sort 99, 100, 101.
Can you take a look at my latest commit, that is based on czxid (as per your suggestion) and I think that makes sense. What ever be the actual id, in the zookeeper what was created recently will be the latest checkpoint. But am not very sure if the checkpointId will really be added in a non-sorted way and can 100 be the latest one (though 101 was also there).

Contributor

ramkrish86 commented May 12, 2017

@tillrohrmann
Thanks for the new PR. I just executed your change with 101, 99 , 100 as the checkpoint order. In this case 100 should be the latest one though the actual ids are not sorted. But with your change and my earlier commit it will always sort 99, 100, 101.
Can you take a look at my latest commit, that is based on czxid (as per your suggestion) and I think that makes sense. What ever be the actual id, in the zookeeper what was created recently will be the latest checkpoint. But am not very sure if the checkpointId will really be added in a non-sorted way and can 100 be the latest one (though 101 was also there).

@ramkrish86

This comment has been minimized.

Show comment
Hide comment
@ramkrish86

ramkrish86 May 12, 2017

Contributor

I won't be available for next 2 to 3 hours. So feel free to decide based on your convenience in case you need to make the RC candidate for 1.3 release. I am sorry that I could not make an initial commit that took care of things properly, should have been more careful. Thanks for the opportunity.

Contributor

ramkrish86 commented May 12, 2017

I won't be available for next 2 to 3 hours. So feel free to decide based on your convenience in case you need to make the RC candidate for 1.3 release. I am sorry that I could not make an initial commit that took care of things properly, should have been more careful. Thanks for the opportunity.

@ramkrish86 ramkrish86 closed this May 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment