Chain APIs for orderer, application, consortium, and channel operations #11

wlahti · 2020-04-28T21:14:20Z

Type of change

Improvement (improvement to code, performance, etc)

Description

First commit:

Chain together API calls to retrieve/update orderer configuration instead of hanging everything off the ConfigTx.

Second commit:

Chain together API calls to retrieve/update application configuration.

Third commit:

Chain together API calls to retrieve/update consortium configuration.

Fourth commit:

Chain together API calls to retrieve/update channel configuration.

Related issues

FAB-17744

caod123 · 2020-04-29T14:17:21Z

@wlahti per CI, looks like theres a couple of linting issues with comments on exported functions

wlahti · 2020-04-30T16:09:15Z

/ci-run

lindluni · 2020-04-30T19:59:35Z

/ci-run

github-actions · 2020-04-30T19:59:49Z

You are not authorized to trigger builds for this pull request!

lindluni · 2020-04-30T20:00:22Z

Will, we enabled the /ci-run command now, so you can use it

github-actions · 2020-04-30T20:00:35Z

You are not authorized to trigger builds for this pull request!

sykesm

I started looking at this fairly closely but decided to step back once I got to the examples.

From my perspective, the examples are the key to all of this. I really want to see examples that show how to use the library. That should drive out the API bits that we need. When I look through the examples, I don't see any references to OriginalConfig. We use a pattern where we create an object that is populated from state in the original config (and we maintain it for our delta computation) but it's never something that someone using the API ever gets.

So, why do we have it? Do we really need it? If we don't need it, then we remove at least one level of chaining that exists already.

The other thing that stands out to me is that we didn't really update the examples to show how chaining would work in practice. All of the calls seem to use the same prefix pattern instead of holding onto the state and modifying the things that need to modified.

Then there are chains like this:

c.UpdatedConfig().Consortiums().Consortium("SampleConsortium").SetChannelCreationPolicy(...)

where I wonder if Consortium(name string) and Consortiums() are the correct API. This style lines up nicely with the REST patterns most of us a used to where the plural form results in a list and the singular form allows a reference to a single instance based on a qualifier.

I know I haven't been involved in the day-to-day of this for a bit so I don't know all of the consideration that went into what's here right now. My ask is that we really put ourselves into the consumer's shoes and see if what's being surfaced is what we'd wand to use.

sykesm · 2020-05-04T12:59:51Z

configtx/capabilities_test.go

@@ -37,13 +37,13 @@ func TestChannelCapabilities(t *testing.T) {
 	err := setValue(config.ChannelGroup, capabilitiesValue(expectedCapabilities), AdminsPolicyKey)
 	gt.Expect(err).NotTo(HaveOccurred())

-	channelCapabilities, err := c.ChannelCapabilities()
+	channelCapabilities, err := c.OriginalConfig().Channel().Capabilities()


Do we need the Config suffix?

sykesm · 2020-05-04T13:04:49Z

configtx/channel.go

+// UpdatedChannelGroup is a ChannelGroup that can be modified in order to
+// generate a config update.
+type UpdatedChannelGroup struct {
+	*ChannelGroup


sykesm · 2020-05-04T13:10:06Z

configtx/channel.go

+	var (
+		err          error
+		consortium   string
+		application  Application
+		orderer      Orderer
+		consortiums  []Consortium
+		capabilities []string
+	)


Instead of 6 vars, should this simply be a Channel and err? Not sure the temporary is buying much since you're just populating the channel object.

Basically, assign each field after getting the data instead of carrying things. That keeps each of the field related blocks together. Given the length of the method, I think that would be easier to read.

sykesm · 2020-05-04T13:15:06Z

configtx/application_test.go

@@ -361,7 +361,7 @@ func TestAddAnchorPeer(t *testing.T) {
 	err = c.AddAnchorPeer("Org2", newOrg2AnchorPeer)
 	gt.Expect(err).NotTo(HaveOccurred())

-	gt.Expect(proto.Equal(c.UpdatedConfig(), expectedUpdatedConfig)).To(BeTrue())
+	gt.Expect(proto.Equal(c.UpdatedConfig().Config, expectedUpdatedConfig)).To(BeTrue())


As a follow-on, I'd suggest writing a matcher and using it.

onsi/gomega#292 (comment)

sykesm · 2020-05-04T13:17:40Z

configtx/capabilities_test.go

@@ -67,13 +67,13 @@ func TestOrdererCapabilities(t *testing.T) {

 	c := New(config)

-	ordererCapabilities, err := c.OrdererCapabilities()
+	ordererCapabilities, err := c.OriginalConfig().Orderer().Capabilities()


I may have said this already in a different section but I think removing the Config suffix from Original and Updated would probably be a good move.

sykesm · 2020-05-04T13:27:36Z

configtx/example_test.go

 	orderer.Kafka.Brokers = []string{"kafka0:9092", "kafka1:9092", "kafka2:9092"}
 	orderer.BatchSize.MaxMessageCount = 500


This is where embedding is good; makes it easy to change the updated version. It's modification of the original that I worry about but it may not be a problem...

sykesm · 2020-05-04T13:28:28Z

configtx/example_test.go

 	if err != nil {
 		panic(err)
 	}

 	orderer.Kafka.Brokers = []string{"kafka0:9092", "kafka1:9092", "kafka2:9092"}
 	orderer.BatchSize.MaxMessageCount = 500

-	err = c.SetOrdererConfiguration(orderer)
+	err = c.UpdatedConfig().Orderer().SetConfiguration(orderer)


The repeated use of the chain does not demonstrate how I'd expect this to be used IRL. Why isn't the orderer reference obtained and maintained across the calls?

sykesm · 2020-05-04T13:29:10Z

configtx/example_test.go

+	err = c.UpdatedConfig().Orderer().Organization("OrdererOrg").RemovePolicy(configtx.WritersPolicyKey)
 	if err != nil {
 		panic(err)
 	}

-	err = c.SetOrdererOrgPolicy("OrdererOrg", configtx.AdminsPolicyKey, "TestPolicy",
+	err = c.UpdatedConfig().Orderer().Organization("OrdererOrg").SetPolicy(


Same here; The first half of the chain is the reference that should be maintained.

sykesm · 2020-05-04T13:29:37Z

configtx/example_test.go

+	err = c.UpdatedConfig().Orderer().RemovePolicy(configtx.WritersPolicyKey)
 	if err != nil {
 		panic(err)
 	}

-	err = c.SetOrdererPolicy(configtx.AdminsPolicyKey, "TestPolicy", configtx.Policy{
+	err = c.UpdatedConfig().Orderer().SetPolicy(configtx.AdminsPolicyKey, "TestPolicy", configtx.Policy{


And here. :) This is an example.

I'll assume this applies to the other occurrences as well.

sykesm · 2020-05-04T13:33:01Z

configtx/example_test.go

 	baseConfig := fetchChannelConfig()
 	c := configtx.New(baseConfig)

-	msp, err := c.OrdererMSP("OrdererOrg")
+	msp, err := c.UpdatedConfig().Orderer().Organization("OrdererOrg").MSP()


It's a little odd for me that, as a user, I'd create a new object from the base config and then have to call UpdatedConfig to get the thing I can actually work with.

So, why is that the case?

wlahti

Refactored the Original/UpdatedConfig logic to simplify things and address your comments.

wlahti · 2020-05-04T19:15:20Z

configtx/config.go

@@ -274,7 +287,7 @@ func newSystemChannelGroup(channelConfig Channel) (*cb.ConfigGroup, error) {

 	err = setPolicies(channelGroup, channelConfig.Policies, AdminsPolicyKey)
 	if err != nil {
-		return nil, fmt.Errorf("failed to set system channel policies: %v", err)
+		return nil, fmt.Errorf("failed to add system channel policies: %v", err)


Good catch. Done.

FAB-17744 Signed-off-by: Will Lahti <[email protected]>

Change from c.Consortiums().Consortium() to c.Consortium(). FAB-17744 Signed-off-by: Will Lahti <[email protected]>

caod123

After some discussion back and forth offline we've come to a better conclusion on this track of work. I'm going to go ahead and merge this as it seems like the direction we're going in with the chaining of the APIs is appropriate and we can iterate more on it to cleanup small nuances

wlahti requested a review from a team as a code owner April 28, 2020 21:14

wlahti force-pushed the fab-17744 branch 2 times, most recently from c0dfc2d to 3ed3691 Compare April 29, 2020 14:04

wlahti force-pushed the fab-17744 branch from 3ed3691 to 4cbf51a Compare April 30, 2020 15:54

wlahti force-pushed the fab-17744 branch 2 times, most recently from f4756c0 to b7c21fc Compare May 1, 2020 14:15

sykesm reviewed May 4, 2020

View reviewed changes

wlahti force-pushed the fab-17744 branch 2 times, most recently from 7202497 to 8a8de75 Compare May 4, 2020 19:14

wlahti commented May 4, 2020

View reviewed changes

wlahti force-pushed the fab-17744 branch 4 times, most recently from 0b68f8c to 63b156d Compare May 6, 2020 20:08

wlahti added 5 commits May 7, 2020 16:16

Chain APIs for orderer operations

fe5993f

FAB-17744 Signed-off-by: Will Lahti <[email protected]>

Chain APIs for application operations

e2a4211

FAB-17744 Signed-off-by: Will Lahti <[email protected]>

Chain APIs for consortium operations

92fed5e

FAB-17744 Signed-off-by: Will Lahti <[email protected]>

Chain APIs for channel operations

c1e56c3

FAB-17744 Signed-off-by: Will Lahti <[email protected]>

Refactor API chaining to remove UpdatedConfig and related types

c15af93

FAB-17744 Signed-off-by: Will Lahti <[email protected]>

wlahti force-pushed the fab-17744 branch from 63b156d to c15af93 Compare May 7, 2020 20:16

caod123 mentioned this pull request May 8, 2020

WIP Chain APIs for pkg/configtx orderer, application, consortium, and channel operations hyperledger/fabric#1166

Closed

Simplify chain for consortium

bcf7f8a

Change from c.Consortiums().Consortium() to c.Consortium(). FAB-17744 Signed-off-by: Will Lahti <[email protected]>

wlahti force-pushed the fab-17744 branch from 3fd4fdc to bcf7f8a Compare May 12, 2020 13:56

caod123 approved these changes May 12, 2020

View reviewed changes

caod123 merged commit 5ffb9e2 into hyperledger:master May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chain APIs for orderer, application, consortium, and channel operations #11

Chain APIs for orderer, application, consortium, and channel operations #11

wlahti commented Apr 28, 2020 •

edited

Loading

caod123 commented Apr 29, 2020

wlahti commented Apr 30, 2020

lindluni commented Apr 30, 2020

github-actions bot commented Apr 30, 2020

lindluni commented Apr 30, 2020

github-actions bot commented Apr 30, 2020

sykesm left a comment

sykesm May 4, 2020

sykesm May 4, 2020

sykesm May 4, 2020

sykesm May 4, 2020

sykesm May 4, 2020

sykesm May 4, 2020

sykesm May 4, 2020

sykesm May 4, 2020

sykesm May 4, 2020

sykesm May 4, 2020

wlahti left a comment

wlahti May 4, 2020

caod123 left a comment

		orderer.Kafka.Brokers = []string{"kafka0:9092", "kafka1:9092", "kafka2:9092"}
		orderer.BatchSize.MaxMessageCount = 500

Chain APIs for orderer, application, consortium, and channel operations #11

Chain APIs for orderer, application, consortium, and channel operations #11

Conversation

wlahti commented Apr 28, 2020 • edited Loading

Type of change

Description

Related issues

caod123 commented Apr 29, 2020

wlahti commented Apr 30, 2020

lindluni commented Apr 30, 2020

github-actions bot commented Apr 30, 2020

lindluni commented Apr 30, 2020

github-actions bot commented Apr 30, 2020

sykesm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlahti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caod123 left a comment

Choose a reason for hiding this comment

wlahti commented Apr 28, 2020 •

edited

Loading