Listen to private data in the events #154

arsulegai · 2021-10-26T08:10:00Z

Listen to the private data map from the deliver response message.
Addresses #153

bestbeforetoday

This approach looks generally OK to me. Please consider the specific review comments I have added an also:

The BlockInfo class is a mashup of two (and now three) different behaviours depending on the event type it represents. This looks to me like BlockInfo should be an interface with three distinct (package private) implementation classes: FilteredBlockInfo, FullBlockInfo, PrivateBlockInfo (or if you prefer maybe BlockWithPrivateDataInfo). BlockInfo just describes the available methods and each implementation class holds only the logic for its block type and we lose all the convoluted if/else method implementations. This would mean BlockEvent should probably also be an interface (extending BlockInfo) with two distinct implementation classes: FullBlockEvent and PrivateBlockEvent. A factory method (on BlockEvent) can be used by the PeerEventServiceClient to create a suitable implementation instance based on the DeliverResponse type.
Some tests are needed. I appreciate this code is not structured to make unit testing easy but you could modify or extend the existing integration testing of events to use the new private type.

bestbeforetoday · 2021-11-10T17:29:29Z

src/main/java/org/hyperledger/fabric/sdk/Channel.java

@@ -1147,7 +1147,7 @@ public PeerOptions getPeersOptions(Peer peer) {
     * @return old options.
     */

-    PeerOptions setPeerOptions(Peer peer, PeerOptions peerOptions) throws InvalidArgumentException {
+    public PeerOptions setPeerOptions(Peer peer, PeerOptions peerOptions) throws InvalidArgumentException {


This method is not currently exposed so I would prefer not to make it public unless absolutely necessary

Given that the peer options is a way to let the SDK request for private data. If one uses the SDK, they should have an ability to set the option. Is there a concern with this exposure?

Alternatives:

Consider a new method to set the peer's block request type option explicitly.

Consider a global setting at the channel level (instead of a peer option), to request generically private data from all the peers.
Let me know your thoughts.

I’m not sure it is the way I would have designed the API but the suggested usage pattern can be seen here:

fabric-sdk-java/src/test/java/org/hyperledger/fabric/sdkintegration/End2endAndBackAgainIT.java

Line 651 in 9c4c3aa

final PeerOptions peerEventingOptions = // we have two peers on one use block on other use filtered

fabric-sdk-java/src/test/java/org/hyperledger/fabric/sdkintegration/UpdateChannelIT.java

Line 489 in 9c4c3aa

peerOptions.registerEventsForFilteredBlocks(); // we need a mix of each type for testing.

Thanks for the test case references. Otherwise, I was heading in the direction to reuse the channel instance created using the gateway SDK.

bestbeforetoday · 2021-11-10T17:42:38Z

src/main/java/org/hyperledger/fabric/sdk/Channel.java

@@ -6416,11 +6416,12 @@ protected void finalize() throws Throwable {
        protected Long startEvents;
        protected Long stopEvents = Long.MAX_VALUE;
        protected boolean registerEventsForFilteredBlocks = false;
+        protected boolean registerEventsForPrivateData = false;


A boolean worked when there were only two options but now there are three I think the event type would be better expressed with an enum. Something like:

public static class PeerOptions { public enum EventType { FILTERED, FULL, PRIVATE } private EventType eventType = EventType.FULL; public EventType getEventType() { return eventType; } public PeerOptions registerEventsForFilteredBlocks() { eventType = EventType.FILTERED; return this; } ... }

bestbeforetoday · 2021-11-10T17:47:38Z

src/main/java/org/hyperledger/fabric/sdk/Channel.java

+         *
+         * @return true if private data blocks will be returned by the peer eventing service.
+         */
+        public boolean isRegisterEventsForPrivateData() {


You could keep the isRegisterEventsForFilteredBlocks() method for backwards compatibility but this one wouldn't be necessary using an enum for the event type

Sure, agree.

bestbeforetoday · 2021-11-10T17:51:36Z

src/main/java/org/hyperledger/fabric/sdk/PeerEventServiceClient.java

@@ -59,6 +59,7 @@

    private final PeerOptions peerOptions;
    private final boolean filterBlock;
+    private final boolean blockAndPrivateData;


Similar to peer options, boolean worked as a toggle between two options but with three options I'd prefer to remove the booleans and use the enum described above

Sure, agree.

bestbeforetoday · 2021-11-10T17:53:39Z

src/main/java/org/hyperledger/fabric/sdk/PeerEventServiceClient.java

+            nso = filterBlock ? broadcast.deliverFiltered(so) :
+                    blockAndPrivateData ? broadcast.deliverWithPrivateData(so) : broadcast.deliver(so);


This would be clearer using an if/else on the event type enum value rather than nested ternary operators

Moved to switch on event type enum.

arsulegai · 2021-11-10T18:54:24Z

Thanks @bestbeforetoday for the detailed review. These comments shows an encouraging sign and vindicates the approach in general. I will get the interfaces/enum added necessary to make the code better readable/cleaner.

bestbeforetoday · 2021-11-12T09:31:43Z

src/main/java/org/hyperledger/fabric/sdk/BlockInfo.java

@@ -31,6 +31,7 @@
 import org.hyperledger.fabric.protos.peer.TransactionPackage;
 import org.hyperledger.fabric.sdk.exception.InvalidProtocolBufferRuntimeException;
 import org.hyperledger.fabric.sdk.transaction.ProtoUtils;
+import sun.jvm.hotspot.utilities.AssertionFailure;


This is failing the build and will prevent it running on non-Oracle/Sun JVMs, so this import cannot be used

Removed. Error on my end, ignored the auto-IDE imports during commit.

arsulegai · 2022-02-12T15:14:27Z

This approach looks generally OK to me. Please consider the specific review comments I have added an also:

The BlockInfo class is a mashup of two (and now three) different behaviours depending on the event type it represents. This looks to me like BlockInfo should be an interface with three distinct (package private) implementation classes: FilteredBlockInfo, FullBlockInfo, PrivateBlockInfo (or if you prefer maybe BlockWithPrivateDataInfo). BlockInfo just describes the available methods and each implementation class holds only the logic for its block type and we lose all the convoluted if/else method implementations. This would mean BlockEvent should probably also be an interface (extending BlockInfo) with two distinct implementation classes: FullBlockEvent and PrivateBlockEvent. A factory method (on BlockEvent) can be used by the PeerEventServiceClient to create a suitable implementation instance based on the DeliverResponse type.

Some tests are needed. I appreciate this code is not structured to make unit testing easy but you could modify or extend the existing integration testing of events to use the new private type.

Thanks for the feedback. I will continue refactoring for the point 1 in upcoming PRs. But extended an existing IT similar to other eventing test case for the private data.

Signed-off-by: S m, Aruna <aruna.mohan@walmart.com>

bestbeforetoday

This change looks pretty good. Thank you for taking the time to rework it.

I had a look at whether it could be refactored to use inheritance or a strategy pattern to provide different implementations based on block event type instead of the cumbersome tests of type and different implementations within each method, but the code in this area looks to be so tangled that this would be a much larger piece of work. So I am happy with this pragmatic approach.

The one change I think might still be worth making is to avoid adding an isBlockAndPrivate() method, leaving the caller to figure out the block type by calling some combination of the is...() methods, and just go ahead and expose a type enum with a getType() method. I've made this change in the last commit on this branch in my fork. Have a look and, if it looks reasonable to you, go ahead and include that change in your PR so I can merge it.

- Expose enum for block event type instead of multiple boolean getters to determine event type. Signed-off-by: Mark S. Lewis <mark_lewis@uk.ibm.com>

arsulegai · 2022-02-18T18:25:58Z

@bestbeforetoday done. Cherry-picked your commit, thanks for helping out. I didn't want to assume why an exception was thrown in earlier case, thought of not breaking much.

On the first point to make this part of code more readable and to use better programming constructs, I am on it and will raise more PRs soon. It's taking some time and didn't want to block the pdc eventing capability until then.

It would also help if we can have a patch release with this feature.

* Listen to private data in the events * Add integration tests for the new PDC events feature * Expose enum for block event type instead of multiple boolean getters to determine event type. Signed-off-by: S m, Aruna <aruna.mohan@walmart.com> Co-authored-by: Mark S. Lewis <mark_lewis@uk.ibm.com>

bestbeforetoday · 2022-02-22T17:42:16Z

This is published in v2.2.12. Many thanks for the contribution!

bestbeforetoday requested changes Nov 10, 2021

View reviewed changes

arsulegai force-pushed the pdc-events branch from 522eaa6 to eb388bf Compare November 10, 2021 18:40

bestbeforetoday reviewed Nov 12, 2021

View reviewed changes

arsulegai force-pushed the pdc-events branch from eb388bf to 3998c14 Compare February 12, 2022 15:15

arsulegai added 2 commits February 13, 2022 00:58

Listen to private data in the events

a7c903f

Signed-off-by: S m, Aruna <aruna.mohan@walmart.com>

Add integration tests for the new PDC events feature

4fe96ba

Signed-off-by: S m, Aruna <aruna.mohan@walmart.com>

arsulegai force-pushed the pdc-events branch from 3998c14 to 4fe96ba Compare February 12, 2022 19:28

arsulegai marked this pull request as ready for review February 12, 2022 19:49

arsulegai requested a review from a team as a code owner February 12, 2022 19:49

arsulegai requested a review from bestbeforetoday February 12, 2022 19:50

arsulegai mentioned this pull request Feb 14, 2022

Feature to listen and report PDC events hyperledger-labs/hlf-connector#3

Merged

bestbeforetoday reviewed Feb 16, 2022

View reviewed changes

Refactor private data events

7d738af

- Expose enum for block event type instead of multiple boolean getters to determine event type. Signed-off-by: Mark S. Lewis <mark_lewis@uk.ibm.com>

arsulegai requested a review from bestbeforetoday February 18, 2022 18:26

bestbeforetoday approved these changes Feb 19, 2022

View reviewed changes

bestbeforetoday merged commit 44edbe5 into hyperledger:main Feb 19, 2022

arsulegai deleted the pdc-events branch February 19, 2022 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Listen to private data in the events #154

Listen to private data in the events #154

arsulegai commented Oct 26, 2021 •

edited

Loading

bestbeforetoday left a comment

bestbeforetoday Nov 10, 2021

arsulegai Nov 10, 2021

bestbeforetoday Nov 10, 2021

arsulegai Feb 9, 2022

bestbeforetoday Nov 10, 2021

arsulegai Feb 9, 2022

bestbeforetoday Nov 10, 2021

arsulegai Feb 9, 2022

bestbeforetoday Nov 10, 2021

arsulegai Feb 9, 2022

bestbeforetoday Nov 10, 2021

arsulegai Feb 9, 2022

arsulegai commented Nov 10, 2021

bestbeforetoday Nov 12, 2021 •

edited

Loading

arsulegai Feb 9, 2022

arsulegai commented Feb 12, 2022

bestbeforetoday left a comment

arsulegai commented Feb 18, 2022

bestbeforetoday commented Feb 22, 2022

		nso = filterBlock ? broadcast.deliverFiltered(so) :
		blockAndPrivateData ? broadcast.deliverWithPrivateData(so) : broadcast.deliver(so);

Listen to private data in the events #154

Listen to private data in the events #154

Conversation

arsulegai commented Oct 26, 2021 • edited Loading

bestbeforetoday 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsulegai commented Nov 10, 2021

bestbeforetoday Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsulegai commented Feb 12, 2022

bestbeforetoday left a comment

Choose a reason for hiding this comment

arsulegai commented Feb 18, 2022

bestbeforetoday commented Feb 22, 2022

arsulegai commented Oct 26, 2021 •

edited

Loading

bestbeforetoday Nov 12, 2021 •

edited

Loading