-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve EthereumListener #1152
base: develop
Are you sure you want to change the base?
Improve EthereumListener #1152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to add all possible use cases of new feature in some sample. I think it's better to create a brand new sample that shows publisher features only.
|
||
@Bean | ||
public EthereumListener ethereumListener(Publisher publisher) { | ||
return publisher.asListener(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking of keeping old listeners and new pub/sub scheme completely decoupled from each other. This is about stable backward compatibility, there is just much smaller chance to break something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to stable backward compatibility we may leave such things like trace()
, onPendingTransactionReceived()
, onBlock(BlockSummary summary)
behind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Do you mean to move backward compatibility logic from Publisher to external adapter class, to keep Publisher code clean?
- For each callback from EthereumListener exists similar Event type, that could be fired and caught.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I mean that it makes sense to keep both old listener which stays unchanged and a publisher. Listener will be marked as deprecated and publisher won't include already deprecated parts like
onPendingTransactionReceived()
method
@@ -290,14 +300,14 @@ public DbFlushManager dbFlushManager() { | |||
} | |||
|
|||
@Bean | |||
public BlockHeaderValidator headerValidator() { | |||
public BlockHeaderValidator headerValidator(SystemProperties systemProperties, Publisher publisher) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it'll be handy in all cases.
Let's say you need to get headerValidator
out of CommonConfig
then you will have to make a tricky call like commonConfig.headerValidator(commonConfig.systemProperties(), commonConfig.publisher())
instead of just commonConfig.headerValidator()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eugene-shevchenko any thoughts on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method uses only in java config, and as I mentioned earlier such bean definition hides component dependencies. With no arguments option I should invoke three factory methods in method's body to get necessary dependencies. But if you think that no-args option is better I can revert method's signature.
PS: It's sad that java config which should configure ApplicationContext
only, we use like object factory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not a bad thing if we want to get rid of Spring one day.
It's more deterministic way.
import java.util.NoSuchElementException; | ||
import java.util.Set; | ||
import java.util.Stack; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to avoid wildcard import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll move back direct class import. It's just IDEA automatic import optimizer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it depends on configuration. You may disable wildcards in idea.
listener.trace(String.format("Block chain size: [ %d ]", this.getSize())); | ||
publisher | ||
.publish(new BlockAddedEvent(summary)) | ||
.publish(new BestBlockAddedEvent(summary, ret == IMPORTED_BEST)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, if we decouple old listener from publisher then BestBlockAddedEvent
won't be needed at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added for backward compatibility. The old code had default callback implementation with data proxing. So to fire such client code in a new fashion we need generate both events.
I agree, that we should leave only one event, when we'll completely remove EthereumListener.
@@ -46,6 +36,9 @@ | |||
import org.springframework.beans.factory.annotation.Autowired; | |||
import org.springframework.stereotype.Component; | |||
|
|||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See wildcard import comment above
|
||
import org.ethereum.core.BlockSummary; | ||
|
||
public class BlockAddedEvent extends Event<BlockSummary> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one and previous should be merged into a single event if we decouple new publisher from old listener
@@ -0,0 +1,4 @@ | |||
package org.ethereum.publish.event; | |||
|
|||
public class NoConnectionsEvent extends SignalEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see any calls that uses this event. Is it ever instantiated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it was created for correspond EthereumListener callback, before all invocations were migrated. I'll remove it.
|
||
import org.ethereum.core.BlockSummary; | ||
|
||
public class BestBlockAddedEvent extends Event<BestBlockAddedEvent.Data> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd omit Event
suffix in each class. Class names like BestBlockAdded
sufficient enough to understand that instantiated object is an event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't argue with that :) I had the same idea too, cause sometimes such naming lead to worst enterprise apps class names. Will rename all events.
* | ||
* @author Eugene Shevchenko | ||
*/ | ||
public interface Single { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to call it OneOffEvent
|
||
import static java.util.Objects.isNull; | ||
|
||
public class EventBus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is better to move experimental
stuff to a separate branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this package completely.
@PostConstruct | ||
public void init() { | ||
publisher.subscribe(to(BlockAdded.class, data -> { | ||
data.getBlockSummary().getBlock().getTransactionsList().forEach(gasPriceTracker::onTransaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic should be somewhere in gasPriceTracker instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, new files lacks License header.
@@ -290,14 +300,14 @@ public DbFlushManager dbFlushManager() { | |||
} | |||
|
|||
@Bean | |||
public BlockHeaderValidator headerValidator() { | |||
public BlockHeaderValidator headerValidator(SystemProperties systemProperties, Publisher publisher) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eugene-shevchenko any thoughts on that?
* @see Publisher | ||
* @see Event | ||
*/ | ||
Publisher subscribe(Subscription subscription); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a user I'd rather expect subscribe(Class type, Consumer handler);
which is clearer. But we have to use generics though to make it really convenient:
<T> Publisher subscribe(Class<? extends Event<T>> type, Consumer<T> handler);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add two overloaded methods:
<T> Publisher subscribe(Class<? extends Event<T>> type, Consumer<T> handler);
<T> Publisher subscribe(Class<? extends Event<T>> type, BiConsumer<T, Subscription.LifeCycle> handler);
but we need current method too, cause sometimes we are in want ofSubscription
instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add some handy links where user could see available type of Events. Current user entrance could stuck without samples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of samples migrated to the new feature, so I guess it's not problem. Also we have utility class Events
for convenience instantiation all of supported events. I'd rather actualize Publisher
javadoc to refer user to all necessary classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this update user was adding listener by extending EthereumListenerAdapter
, he got the view of all available listening options in one click. Since this update he should write .subscribe(
and what's next? Samples are not the answer, not everyone can find them and you will not go to samples every time you need to add listener, Subscription class looks difficult from the first view, events are different classes in some package somewhere, not listed in one place. So the idea is that main entrance should look handy and simplify user's usage. I'm not sure what's the best way to do it but current entrance could stuck most of users.
@Override | ||
public void onNodeDiscovered(Node node) { | ||
compositeListener.onNodeDiscovered(node); | ||
publisher.publish(Events.onNodeDiscovered(node)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use static import for Events
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, we have several methods with the same signature in both classes. That's why we should use full name in this case. In addition it's temporary class, so it's not critical I suppose.
private final List<EthereumListener> listeners; | ||
|
||
|
||
public CompositeEthereumListener(Executor executor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have anything to update in deprecated class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly field injection is bad practice cause it hides component dependencies. But in this case it's not critical because class marked as deprecated.
I moved bean definition to java config to highlight that old and new components use the same Executor
. And then both Publisher
and deprecated CompositeEthereumListener
injects into BackwardCompatibilityEthereumListenerProxy
. IMHO it's more obvious and helps avoid dependency mess.
This also applies to the previous comment.
@@ -150,4 +160,86 @@ default void onBlock(BlockSummary blockSummary, boolean best) { | |||
void onTransactionExecuted(TransactionExecutionSummary summary); | |||
|
|||
void onPeerAddedToSyncPool(Channel peer); | |||
|
|||
EthereumListener STUB = new EthereumListener() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this either necessary. Let it use old deprecated EthereumListenerAdapter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll replace it with EthereumListenerAdapter
@@ -38,7 +38,7 @@ | |||
* (if {@link #getPercentileShare()} is not overridden) of the latest transactions were | |||
* executed at this or lower price. | |||
*/ | |||
public class RecommendedGasPriceTracker extends EthereumListenerAdapter { | |||
public class RecommendedGasPriceTracker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks backward compatibility. We also need to introduce a mechanism for easy subscribing to the new publisher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone use this class except us? I thought that this class for our internal logic only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We recommend to use this class in 'official' docs:
* want to get accurate recommended gas price use {@link org.ethereum.listener.RecommendedGasPriceTracker} |
If it's not pretty necessary we should preserve backward compatibility, to not force EthJ lib users to rewrite their code after each major update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way we use this class nowhere =)
public BlockAdded(BlockSummary blockSummary, boolean best) { | ||
super(new Data(blockSummary, best)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line
* | ||
* @author Eugene Shevchenko | ||
*/ | ||
public class LifeCycleSubscription<E extends Event<D>, D> extends Subscription<E, D> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet we don't need yet another abstraction here. Introducing it makes an API less obvious for users. I am more about to have two constructors for Subscription
, one with BiConsumer
, another one with Consumer
that is extended to BiConsumer
during construction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll move this functionality to Subscription
*/ | ||
public class LifeCycleSubscription<E extends Event<D>, D> extends Subscription<E, D> { | ||
|
||
public static class LifeCycle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkalinin Sorry, I removed your comment accidentally
*/ | ||
public class LifeCycleSubscription<E extends Event<D>, D> extends Subscription<E, D> { | ||
|
||
public static class LifeCycle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkalinin Sorry, I removed your comment accidentally
@@ -290,14 +300,14 @@ public DbFlushManager dbFlushManager() { | |||
} | |||
|
|||
@Bean | |||
public BlockHeaderValidator headerValidator() { | |||
public BlockHeaderValidator headerValidator(SystemProperties systemProperties, Publisher publisher) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not a bad thing if we want to get rid of Spring one day.
It's more deterministic way.
@@ -209,11 +223,6 @@ public BlockchainImpl withAdminInfo(AdminInfo adminInfo) { | |||
return this; | |||
} | |||
|
|||
public BlockchainImpl withEthereumListener(EthereumListener listener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Listener doesn't look the thing which is required for running BlockchainImpl
. Also old constructor could be used by somebody. I'd prefer to keep both old constructor (and init listener there with EthereumListener.EMPTY
) and withEthereumListener
method.
public void addListener(EthereumListener listener) { | ||
logger.info("Ethereum listener added"); | ||
((CompositeEthereumListener) this.listener).addListener(listener); | ||
((BackwardCompatibilityEthereumListenerProxy) listener).getCompositeListener().addListener(listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user have used our listener in most common way (ethereum.addListener(new EthereumListenerAdapter(){})
) he will get strange exception here, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think so ? This code fully support old logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users have code like
ethereum.addListener(new EthereumListenerAdapter());
after that it goes:
->
worldManager.addListener(listener)
->
((BackwardCompatibilityEthereumListenerProxy) EthereumListenerAdapter instance)
import org.ethereum.publish.event.message.MessageSent; | ||
import org.ethereum.publish.event.message.PeerHandshaked; | ||
|
||
public final class Events { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this class made for internal purposes of compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this class added for using convenience. For instance using
publisher.publish(onSyncDone(SOME_STATUS))
more readable than publisher.publish(new SyncDone(SOME_STATUS))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it
* @see Publisher | ||
* @see Event | ||
*/ | ||
Publisher subscribe(Subscription subscription); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add some handy links where user could see available type of Events. Current user entrance could stuck without samples.
@@ -99,6 +104,10 @@ public void channelRead0(final ChannelHandlerContext ctx, EthMessage msg) throws | |||
msgQueue.receivedMessage(msg); | |||
} | |||
|
|||
public Publisher getPublisher() { | |||
return ((BackwardCompatibilityEthereumListenerProxy) ethereumListener).getPublisher(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think BackwardCompatibilityEthereumListenerProxy
should be localized somehow and don't spread to such classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main idea of BackwardCompatibilityEthereumListenerProxy
is to encapsulate new and old publishing mechanisms under EthereumListener
interface, and proxy old messages to both of them with / without transformation. We should use this component everywhere to reduce the number of changes and support both mechanisms at the same time.
BackwardCompatibilityEthereumListenerProxy
will simplify removing deprecated logic in the future. And when the time comes we'll remove BackwardCompatibilityEthereumListenerProxy
and all EthereumListener
related stuff and leave only Publisher
invokes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this idea as we are moving everything inside our application to the new subscribe-publish model, while leaving common user interfaces and marking them as deprecated. So any backward compatibility gateways should be localized to old public user interfaces.
} | ||
}); | ||
// when block arrives look for our included transactions | ||
ethereum.subscribe(to(BlockAdded.class, this::onBlock)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see old method usage in private void replayOnly() throws Exception
ethereum.addListener(blockReplay);
} | ||
|
||
@Override | ||
public void trace(String output) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this kind of event was not added to publisher-subscriber implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cause we don't want to support it anymore
@Override | ||
public void onBlock(BlockSummary blockSummary) { | ||
compositeListener.onBlock(blockSummary); | ||
publisher.publish(Events.onBlockAdded(blockSummary)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a new pub/sub there should not be a version of onBlockAdded()
which doesn't accept best
class. Treat it as a deprecated feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still requesting to remove Events.onBlockAdded(blockSummary)
shortcut and corresponding event from new pub/sub.
|
||
public class SyncDone extends Event<SyncDone.State> implements OneOffEvent { | ||
|
||
public enum State { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose moving this enum to SyncManager
class or create a standalone enum in sync
package
*/ | ||
public class PendingTransactionUpdated extends Event<PendingTransactionUpdated.Data> { | ||
|
||
public enum State { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move it to PendingTransaction
class
this.subscription = subscription; | ||
} | ||
|
||
public void unsubscribe() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lack of description that points to potential use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still not added :)
/** | ||
* More convenient version of {@link #subscribe(Subscription)} | ||
*/ | ||
<T> Publisher subscribe(Class<? extends Event<T>> type, BiConsumer<T, Subscription.LifeCycle> handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Publisher
class should also have such shortcuts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To increase UX we may create shortcuts for event classes and put them together into one place.
For example, use Event
or create new EventType
class and add a list of event types definition with readable names:
public static final Class<? extends Event<BlockAdded.Data>> BLOCK_ADDED = BlockAdded.class;
Then point this type aggregation class in javadocs. It would work like an event picker then and make user choice faster, user will have to type Event.
to get a list of available options
…couple bugs. Refactors samples.
@@ -44,7 +51,7 @@ | |||
|
|||
List<AbstractCachedSource<byte[], ?>> writeCaches = new CopyOnWriteArrayList<>(); | |||
List<Source<byte[], ?>> sources = new CopyOnWriteArrayList<>(); | |||
Set<DbSource> dbSources = new HashSet<>(); | |||
Set<DbSource> dbSources ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for me there is no reason for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll revert this change
gLogger.info("EthereumJ node started: enode://" + toHexString(config.nodeId()) + "@" + config.externalIp() + ":" + config.listenPort()); | ||
} | ||
|
||
@PostConstruct | ||
public void init() { | ||
worldManager.subscribe(to(BLOCK_ADED, data -> gasPriceTracker.onBlock(data.getBlockSummary()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BLOCK_ADED
=> BLOCK_ADDED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix it
} | ||
|
||
@Override | ||
public void trace(String output) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cause we don't want to support it anymore
@Override | ||
public void onBlock(BlockSummary blockSummary) { | ||
compositeListener.onBlock(blockSummary); | ||
publisher.publish(Events.onBlockAdded(blockSummary)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still requesting to remove Events.onBlockAdded(blockSummary)
shortcut and corresponding event from new pub/sub.
* <p> | ||
* Created by Eugene Shevchenko on 07.10.2018. | ||
*/ | ||
public class BlockReplayer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mark its predecessor BlockReplay
class as a deprecated one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I'll add comment.
this.subscription = subscription; | ||
} | ||
|
||
public void unsubscribe() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still not added :)
This PR covers issue #1138.
Adds Publisher abstraction that implements pub/sub model to deliver events from publisher to subscriber.
Replaces CompositeEthereumListener / EthereumListenerAdaptor / EthereumListener using with new event publishing mechanism.
Keeps backward compatibility with old event handling model.