Skip to content
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

WIP: Cancel message #370

Open
wants to merge 13 commits into
base: devel
Choose a base branch
from
Open

WIP: Cancel message #370

wants to merge 13 commits into from

Conversation

LipuFei
Copy link
Contributor

@LipuFei LipuFei commented Sep 9, 2014

Use cancel message to replace undo message.
All undo message code remain the same.

@LipuFei
Copy link
Contributor Author

LipuFei commented Sep 9, 2014

@whirm I'll remove the pep8 commit for now, otherwise the PR is too difficult to review.

@tribler-ci
Copy link

Test FAILED.
Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org/job/GH_Dispersy_pull-request-tester_devel/784/

@whirm
Copy link
Contributor

whirm commented Sep 9, 2014

@LipuFei yes, those should always go alone

@NielsZeilemaker
Copy link
Contributor

@LipuFei why are you adding the cancel to the permission triplet? That shouldn't be neccesary. You authorise someone to send a dispersy_cancel message etc.

@LipuFei
Copy link
Contributor Author

LipuFei commented Sep 10, 2014

@NielsZeilemaker I made this PR to make it easy for us to review. Thanks! I'll check everything again.

@LipuFei
Copy link
Contributor Author

LipuFei commented Sep 10, 2014

@NielsZeilemaker What does the permission_triplets mean?

@NielsZeilemaker
Copy link
Contributor

Basically, you can authorize/revoke or grant someone the permission to create a message of a certain type. I wouldn't start messing with this as its never tested/understood by me.
Moreover, are you sure you want to introduce a cancel callback? Maybe calling the undo callback would suffice. Chaning it slightly to allow you to pass a cancelled/undone/done variable instead of the undone/done currently used.

@NielsZeilemaker
Copy link
Contributor

Btw, the documentation regarding the permissions should still be correct. You can have a look there.

@LipuFei
Copy link
Contributor Author

LipuFei commented Sep 10, 2014

@NielsZeilemaker I had thought about it and I decided to duplicate the undo stuff instead of messing them up. Later, when we want to remove the undo, we can simply remove every undo-related thing without worry about causing damage to the cancel message. I will read the permission docs. Thanks!

for message in community.get_meta_messages():
# grant all permissions for messages that use LinearResolution or DynamicResolution
if isinstance(message.resolution, (LinearResolution, DynamicResolution)):
for allowed in (u"authorize", u"revoke", u"permit"):
for allowed in (u"authorize", u"revoke", u"permit", u"cancel"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the cancel here, but leave it in timeline.py. E.g. add it when the "undo" thingy is there.

@tribler-ci
Copy link

Test FAILED.
Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org/job/GH_Dispersy_pull-request-tester_devel/785/

@LipuFei LipuFei force-pushed the cancel_message branch 2 times, most recently from 9d0f83b to b6cd526 Compare September 10, 2014 17:18
@tribler-ci
Copy link

Test FAILED.
Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org/job/GH_Dispersy_pull-request-tester_devel/786/

@tribler-ci
Copy link

Test FAILED.
Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org/job/GH_Dispersy_pull-request-tester_devel/787/

@LipuFei
Copy link
Contributor Author

LipuFei commented Sep 10, 2014

@whirm I guess this is more or less it. I will check the unit tests one more time after my presentation rehearsal. Also, before we merge, we should change the AllChannel community to use this cancel message and test on jenkins.

@whirm
Copy link
Contributor

whirm commented Sep 11, 2014

Ok, let's review it later

@tribler-ci
Copy link

Merged build triggered.

@tribler-ci
Copy link

Merged build started.

@tribler-ci
Copy link

Merged build finished.

@tribler-ci
Copy link

Test FAILed.
Refer to this link for build results: http://jenkins.tribler.org/job/GH_Dispersy_pull-request-tester_devel/836/

@tribler-ci
Copy link

Merged build triggered.

@tribler-ci
Copy link

Merged build started.

@tribler-ci
Copy link

Merged build finished.

@tribler-ci
Copy link

Test FAILed.
Refer to this link for build results: http://jenkins.tribler.org/job/GH_Dispersy_pull-request-tester_devel/837/

@tribler-ci
Copy link

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Dispersy_pull-request-tester_devel/848/
Test FAILed.

@LipuFei
Copy link
Contributor Author

LipuFei commented Apr 7, 2015

@NielsZeilemaker One of the cancel message tests didn't pass. The test does the following:

Three nodes: SELF, NODE, OTHER
What they do:
1. SELF gives NODE the permission to cancel.
2. OTHER created message M1.
3. NODE cancels M1 with cancel message C1.
4. M1 should be found as cancelled by NODE (This one passed).
5. SELF revokes NODE's cancel permission.
6. M1 should still be marked as cancelled by NODE (This one failed).

This test was passing before. Am I doing it correctly? If a user's cancel permission has been revoked, should his old cancel messages be regarded as invalid as well?

@NielsZeilemaker
Copy link
Contributor

@LipuFei maybe create a wiki page describing the cancel message, and what it should do. Then afterwards, I can have a look and maybe we can modify the behaviour of your implementation accordingly.
Also describe situations such as the one above. A gets the permission to cancel other, A's permission gets revoke -> what should be do with the messages cancelled by A.

@LipuFei
Copy link
Contributor Author

LipuFei commented Apr 7, 2015

@NielsZeilemaker I've made a wiki page: https://github.com/Tribler/dispersy/wiki/Cancel-Message. Hope it explains.

@NielsZeilemaker
Copy link
Contributor

@LipuFei I made some changes to the document. I guess cancel messages should also not cancel an authorize message right? Or are we adding that? Additionally, we should think about being able to drop cancelled messages from the database in order to save space.

@tribler-ci
Copy link

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Dispersy_pull-request-tester_devel/864/
Test FAILed.

@tribler-ci
Copy link

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Dispersy_pull-request-tester_devel/871/
Test FAILed.

@tribler-ci
Copy link

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Dispersy_pull-request-tester_devel/886/
Test FAILed.

@tribler-ci
Copy link

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Dispersy_pull-request-tester_devel/891/
Test FAILed.

@tribler-ci
Copy link

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Dispersy_pull-request-tester_devel/894/
Test FAILed.

@LipuFei
Copy link
Contributor Author

LipuFei commented Sep 14, 2015

Hi @NielsZeilemaker, could you review this PR again? Thanks!

@whirm whirm changed the title Cancel message WIP: Cancel message Apr 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants