Added state indicating when the spark master changed #9

Merged
merged 1 commit into from Oct 3, 2016

Conversation

Projects
None yet
2 participants
Contributor

petevg commented Oct 3, 2016

+1

Owner

johnsca commented Oct 3, 2016

PR cleaned up to remove extraneous changes.

requires.py
+ return conv.get_remote('master')
+
+ def is_scaled(self):
+ return len(self.conversation().units) > 1
@petevg

petevg Oct 3, 2016

Contributor

Shouldn't there be a set_remote('this node is the master') somewhere in here? I thought that was one of the things that you wanted to keep from the old PR ...

@johnsca

johnsca Oct 3, 2016

Owner

That, and actually is_scaled, were artifacts of the "subordinate to spark" approach and aren't used now. The "this node is master" doesn't even make sense in the non-subordinate case because the relation may represent multiple remote units.

On the flip side, the is_scaled check here wouldn't make sense or work in the subordinate case, because I think it would only see the single unit it's attached to. I should probably remove this as well.

@petevg

petevg Oct 3, 2016

Contributor

Got it. I am +1 after is_scaled goes away, then :-)

requires.py
+ return conv.get_remote('master')
+
+ def is_scaled(self):
+ return len(self.conversation().units) > 1
@petevg

petevg Oct 3, 2016

Contributor

Shouldn't there be a set_remote('this node is the master') somewhere in here? I thought that was one of the things that you wanted to keep from the old PR ...

@johnsca

johnsca Oct 3, 2016

Owner

That, and actually is_scaled, were artifacts of the "subordinate to spark" approach and aren't used now. The "this node is master" doesn't even make sense in the non-subordinate case because the relation may represent multiple remote units.

On the flip side, the is_scaled check here wouldn't make sense or work in the subordinate case, because I think it would only see the single unit it's attached to. I should probably remove this as well.

@petevg

petevg Oct 3, 2016

Contributor

Got it. I am +1 after is_scaled goes away, then :-)

Contributor

petevg commented Oct 3, 2016

LGTM/+1

@johnsca johnsca merged commit 24f7ce5 into master Oct 3, 2016

@kwmonroe kwmonroe deleted the spark-master-change branch Oct 3, 2016

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