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

Enable/Disable filter blocks events register #75

Closed
wants to merge 3 commits into from

Conversation

rcangiamila
Copy link

Implemented new feature to enable/disable filter blocks events register.

@rcangiamila rcangiamila requested a review from a team as a code owner April 20, 2021 16:49
Roberto Cangiamila added 3 commits April 20, 2021 19:12
Signed-off-by: Roberto Cangiamila <roberto.cangiamila@faberbee.com>
Signed-off-by: Roberto Cangiamila <roberto.cangiamila@faberbee.com>
Signed-off-by: Roberto Cangiamila <roberto.cangiamila@faberbee.com>
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution and for making the effort to include unit tests.

However... as discussed by email, this approach is not consistent with the support for different event types in the Node SDK. It also does not work for peers obtained by service discovery. I have added the email discussion as a comment in the Jira.

If you want to use the approach taken in this PR, I think you can probably do it without code changes in fabric-sdk-java by programmatically removing and re-adding (with appropriate peer options) all the peers from the underlying channel immediately after obtaining the network, similar to the implementation of ReplayListenerSession.

@@ -102,7 +102,7 @@

<!-- Checks for imports -->
<!-- See https://checkstyle.org/config_import.html -->
<module name="AvoidStarImport"/>
<!-- <module name="AvoidStarImport"/>-->
Copy link
Member

Choose a reason for hiding this comment

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

Please don't change the Checkstyle rules to match your code submission. They are there to maintain consistent style and quality across the project. Instead, deliver code that satisfies the Checkstyle rules

@@ -7,7 +7,7 @@

<groupId>org.hyperledger.fabric</groupId>
<artifactId>fabric-gateway-java</artifactId>
<version>2.2.0</version>
<version>2.2.2</version>
Copy link
Member

Choose a reason for hiding this comment

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

We are not publishing from the main branch so the version here doesn't need to be changed, although there's no harm in doing that. It's just unnecessary noise in the commit history

import org.hyperledger.fabric.sdk.exception.InvalidArgumentException;
import org.hyperledger.fabric.sdk.exception.ProposalException;
import org.hyperledger.fabric.sdk.exception.ServiceDiscoveryException;
import org.hyperledger.fabric.sdk.transaction.TransactionContext;

import java.util.ArrayList;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really bothered by import ordering but this file change only changes import ordering and contains no code changes so is unnecessary

@bestbeforetoday
Copy link
Member

Please also reference the Jira in the first line of the PR commit message, for example:

FGJ-107: Enable/Disable filter blocks events register

This allows Jira to associate Git commits with Jira issues.

@bestbeforetoday
Copy link
Member

This has been stale for a long time so closing. Feel free to reopen or raise a new PR if you want to progress this.

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

Successfully merging this pull request may close these issues.

None yet

2 participants