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

Sequencer entity #546

Merged
merged 7 commits into from
Apr 11, 2017
Merged

Sequencer entity #546

merged 7 commits into from
Apr 11, 2017

Conversation

grkvlt
Copy link
Member

@grkvlt grkvlt commented Jan 31, 2017

An entity and a group that manage sequential sensor values

Copy link
Contributor

@aledsage aledsage left a comment

Choose a reason for hiding this comment

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

I can guess at why these are useful (e.g. some clusters require us to give each member an id that are sequential numbers). However, it would be very useful if you could give us some concrete use-cases so that we understand why these are being added to Brooklyn.

If that is the use-case, I'd prefer the effector approach, with DSL support to invoke the effector and format the result. That feels more explicit compared to the SequenceGroup. But it really depends what use-cases you're envisaging.

public void setUp() throws Exception {
super.setUp();

app = TestApplication.Factory.newManagedInstanceForTests();
Copy link
Contributor

Choose a reason for hiding this comment

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

super-class will have instantiated app - don't do it here, and delete the private field app. Also delete tearDown as that is done by super.

group.setEntityFilter(EntityPredicates.hasInterfaceMatching(".*TestEntity"));

assertEqualsIgnoringOrder(group.getMembers(), ImmutableSet.of(e1, e2, e3));
assertAttributeEquals(e1, value, 12345);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be sure that e1, e2, e3` will be the order of the members? Is that because it is called sequentially with the order that the entities were created?

String currentString();

@Effector(description = "Update and return the next numeric value of the sequence")
Integer nextValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer we follow the naming conventions of AtomicInteger - so rename increment to incrementAndGet, and delete nextValue + nextString.

I'm inclined to let the caller do the formatting of the result, rather than having separate nextValue and nextString. The difference between those two methods, and their interaction, feels as though it might confuse.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, that would make things more obvious and simpler.

@aledsage
Copy link
Contributor

aledsage commented Feb 7, 2017

@grkvlt (cc @drigodwin) given we have DynamicCluster.CLUSTER_MEMBER_ID, where do we need this sequencer entity as opposed to just using that?

@grkvlt
Copy link
Member Author

grkvlt commented Feb 9, 2017

@aledsage the use case in mind was to sequentially number across multiple clusters (i.e a fabric spread around several locations) which needed the implicit, SequenceGroup-based mechanism. However the explicit (effector based) SequenceEntity version also seemed useful, so I left it in.

@grkvlt
Copy link
Member Author

grkvlt commented Feb 9, 2017

@drigodwin we need to document this, I will add some descriptive text explaining this for Brooklyn docs

@ahgittin
Copy link
Contributor

I feel like this will soon be superseded, by being able to point at some shared-scoped atomic integer and request a getAndIncrement directly in YAML as part of start. Haven't looked in detail but if that impl could be done as an extension of DynamicGroup could the code here be simplified?

That said not that opposed to adding for now if useful with an expectation it might fall away.

@grkvlt
Copy link
Member Author

grkvlt commented Mar 28, 2017

@mikezaccardo is this still useful, e.g. for HLF blueprints?

@mikezaccardo
Copy link
Contributor

mikezaccardo commented Apr 10, 2017

@grkvlt this would indeed be useful for the Hyperledger Fabric multi-cluster in place of this entity: https://github.com/cloudsoft/brooklyn-hyperledger/blob/c85c215d725017b787c133c1483a21f4e9433b09/catalog/hyperledger/multi-cluster.bom#L205-L251

Copy link
Member

@drigodwin drigodwin left a comment

Choose a reason for hiding this comment

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

I think this is useful as @mikezaccardo has highlighted so I'm going to merge this now.

@asfgit asfgit merged commit 62989ac into apache:master Apr 11, 2017
asfgit pushed a commit that referenced this pull request Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants