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

Add missing got_static_announcement #302

Merged
merged 1 commit into from Aug 24, 2016

Conversation

david415
Copy link
Member

@david415 david415 commented Aug 2, 2016

i copy pasted this snippit of code from my dev branch introless-multiintro_yaml_config.2
it's probably wrong... and needs tests; DO NOT MERGE

@coveralls
Copy link
Collaborator

coveralls commented Aug 2, 2016

Coverage Status

Coverage decreased (-0.04%) to 92.231% when pulling 8f04b9a on david415:2801.fix_connections_yaml.0 into 6f8c96e on tahoe-lafs:master.

@coveralls
Copy link
Collaborator

coveralls commented Aug 22, 2016

Coverage Status

Coverage decreased (-0.003%) to 92.16% when pulling 327d70f on david415:2801.fix_connections_yaml.0 into 85cf1d6 on tahoe-lafs:master.

self.tubs[serverid].setServiceParent(self)

def got_static_announcement(self, key_s, ann):
if key_s is not None:
Copy link
Member

@warner warner Aug 22, 2016

Choose a reason for hiding this comment

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

key_s should always be present now (since we got rid of v1 introducers). Let's remove the if and do these precondition checks all the time. Let's also make the assert on the service-name into a precondition(). (we can make this change to _got_announcement() too)

@warner
Copy link
Member

warner commented Aug 22, 2016

I kind of liked the approach we had before, where NativeStorageServer created it's own Tub internally. Why move Tub creation up to the StorageFarmBroker? (in the future, we'll add HTTPStorageServer, which won't need Tubs at all, so it seems cleaner to do the Tub creation inside the class that knows it needs one). I'll have to think about this.. I can see that there's a question of what happens when a server is updated (e.g. we get an announcement for a server that we've already started using, so there's already a Tub for it: having the StorageFarmBroker own the Tub would make it easier to keep using the same Tub even though the NativeStorageServer gets replaced).

Is there any way we can merge got_static_announcement with the existing _got_announcement? There's a lot of code duplication between them, and I think merging them would teach us how to get the ownership/updating part right. got_static_announcement is lacking the s.on_status_changed hook that helps Magic Folders know when to start, the if old clause shouldn't ever fire for static announcements because they're added before anything else has been added. Except for the self.static_servers tracking, I don't think I see anything that would prevent got_static_announcement from just immediately delegating to self._got_announcement.

Please rebase to current master when you get a chance, this overlaps a couple of moderate-sized branches from the last month.

@coveralls
Copy link
Collaborator

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-0.02%) to 92.148% when pulling 1f8e4c8 on david415:2801.fix_connections_yaml.0 into 61eb839 on tahoe-lafs:master.

@david415
Copy link
Member Author

ok i've made the corrections you specified and removed code duplication. please review

@warner
Copy link
Member

warner commented Aug 24, 2016

Some more items:

  • the new diff removed the old.disownServiceParent() from the replace-an-old-server case, and removed the s.setServiceParent() from the new-server case. I think both need to be reinstated. Do a git diff master..HEAD and make sure that nothing's been changed that wasn't necessary. The name change from serverid to server_id could be put into a separate patch, but I think it's ok to have it combined into the rest of the patch too, no big deal.
  • please rebase to current master and force-push to this branch.
  • consider squashing all of this. in fact, maybe you should just extract a diff and then make a new branch that applies the entire diff as a single patch:
git checkout 2801.fix_connections_yaml.0
git diff master..HEAD >p
git branch new master
git checkout new
patch -p1 <p
git add, git commit
git diff new..2801.fix_connections_yaml.0 # this should be empty
git checkout 2801.fix_connections_yaml.0
git reset --hard new
git push -f origin 2801.fix_connections_yaml.0

That might be easier than rebasing this, and will result in the same code.

@coveralls
Copy link
Collaborator

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-0.0008%) to 92.163% when pulling 78e77c3 on david415:2801.fix_connections_yaml.0 into 61eb839 on tahoe-lafs:master.

@warner
Copy link
Member

warner commented Aug 24, 2016

looks good.. just waiting for tests to complete, then I'll land it. thanks!

@warner warner merged commit de61cd2 into tahoe-lafs:master Aug 24, 2016
@coveralls
Copy link
Collaborator

Coverage Status

Coverage decreased (-0.008%) to 92.156% when pulling de61cd2 on david415:2801.fix_connections_yaml.0 into 61eb839 on tahoe-lafs:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants