You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/04/09 09:05:48 UTC
[GitHub] [ignite] ololo3000 opened a new pull request #7647: IGNITE-12879
Changes DiscoveryHook class methods naming.
ololo3000 opened a new pull request #7647: IGNITE-12879 Changes DiscoveryHook class methods naming.
URL: https://github.com/apache/ignite/pull/7647
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ololo3000 commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409426799
##########
File path: modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TestTcpDiscoverySpi.java
##########
@@ -44,4 +54,24 @@
@Override public void simulateNodeFailure() {
super.simulateNodeFailure();
}
+
+ /** {@inheritDoc} */
+ @SuppressWarnings("ZeroLengthArrayAllocation")
+ @Override public void setListener(@Nullable DiscoverySpiListener lsnr) {
+ super.setListener(lsnr == null || discoHooks.isEmpty() ?
Review comment:
> It seems to me that we can just pass discoHooks into the listener.
> Therefore list always be up to date inside the listener.
It's not clear for me in what cases hooks are needed to be modified after SPI start. I think that the current approach is more robust.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ololo3000 commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409404712
##########
File path: modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TestTcpDiscoverySpi.java
##########
@@ -44,4 +54,24 @@
@Override public void simulateNodeFailure() {
super.simulateNodeFailure();
}
+
+ /** {@inheritDoc} */
+ @SuppressWarnings("ZeroLengthArrayAllocation")
+ @Override public void setListener(@Nullable DiscoverySpiListener lsnr) {
+ super.setListener(lsnr == null || discoHooks.isEmpty() ?
Review comment:
addDiscoveryHook method asserts that it must be called only before TcpDiscoverySpi start. So it's impossible to use addDiscoveryHook after listener is set.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] NSAmelchev commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r406403390
##########
File path: modules/core/src/test/java/org/apache/ignite/testframework/GridTestUtils.java
##########
@@ -164,32 +169,89 @@ public void ignite(IgniteEx ignite) {
}
}
+ /**
+ * Extended {@link DiscoveryHook} for {@link DiscoveryCustomMessage} interception.
+ */
+ public static class DiscoveryCustomMessageHook extends DiscoveryHook {
+ /** {@inheritDoc} */
+ @Override public void beforeDiscovery(DiscoverySpiCustomMessage msg) {
+ if (!(msg instanceof CustomMessageWrapper))
+ return;
+
+ beforeDiscovery(unwrap((CustomMessageWrapper)msg));
+ }
+
+ /**
+ * Handles {@link DiscoveryCustomMessage}.
+ *
+ * @param customMsg Intercepted {@link DiscoveryCustomMessage}.
+ */
+ public void beforeDiscovery(DiscoveryCustomMessage customMsg) {
+ // No-op.
+ }
+
+ /** {@inheritDoc} */
+ @Override public void afterDiscovery(DiscoverySpiCustomMessage msg) {
+ if (!(msg instanceof CustomMessageWrapper))
+ return;
+
+ afterDiscovery(unwrap((CustomMessageWrapper)msg));
+ }
+
+ /**
+ * Handles {@link DiscoveryCustomMessage}.
+ *
+ * @param customMsg Intercepted {@link DiscoveryCustomMessage}.
+ */
+ public void afterDiscovery(DiscoveryCustomMessage customMsg) {
+ // No-op.
+ }
+
+ /**
+ * Obtains {@link DiscoveryCustomMessage} from {@link CustomMessageWrapper}.
+ *
+ * @param wrapper Wrapper of {@link DiscoveryCustomMessage}.
+ */
+ private DiscoveryCustomMessage unwrap(CustomMessageWrapper wrapper) {
+ return U.field(wrapper, "delegate");
+ }
+ }
+
/**
* Injects {@link DiscoveryHook} into handling logic.
*/
public static final class DiscoverySpiListenerWrapper implements DiscoverySpiListener {
/** */
private final DiscoverySpiListener delegate;
- /** */
- private final DiscoveryHook hook;
+ /** Interceptors of discovery messages. */
+ private final DiscoveryHook[] hooks;
/**
* @param delegate Delegate.
- * @param hook Hook.
+ * @param hooks Interceptors of discovery messages.
*/
- private DiscoverySpiListenerWrapper(DiscoverySpiListener delegate, DiscoveryHook hook) {
- this.hook = hook;
+ private DiscoverySpiListenerWrapper(DiscoverySpiListener delegate, DiscoveryHook[] hooks) {
+ this.hooks = hooks;
this.delegate = delegate;
}
/** {@inheritDoc} */
- @Override public IgniteFuture<?> onDiscovery(int type, long topVer, ClusterNode node, Collection<ClusterNode> topSnapshot, @Nullable Map<Long, Collection<ClusterNode>> topHist, @Nullable DiscoverySpiCustomMessage spiCustomMsg) {
- hook.handleDiscoveryMessage(spiCustomMsg);
+ @Override public IgniteFuture<?> onDiscovery(
+ int type,
+ long topVer,
+ ClusterNode node,
+ Collection<ClusterNode> topSnapshot,
+ @Nullable Map<Long, Collection<ClusterNode>> topHist,
+ @Nullable DiscoverySpiCustomMessage spiCustomMsg
+ ) {
+ for (DiscoveryHook hook : hooks)
+ hook.beforeDiscovery(spiCustomMsg);
IgniteFuture<?> fut = delegate.onDiscovery(type, topVer, node, topSnapshot, topHist, spiCustomMsg);
- hook.afterDiscovery(spiCustomMsg);
+ for (DiscoveryHook hook : hooks)
+ hook.afterDiscovery(spiCustomMsg);
Review comment:
The `afterDiscovery` will be called not after discovery notifing listeners. `delegate.onDiscovery` is async.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] NSAmelchev commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r406303049
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/continuous/IgniteContinuousQueryMetadataUpdateTest.java
##########
@@ -57,11 +55,26 @@
@Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
- TestDiscoverySpi discoSpi = new TestDiscoverySpi();
+ TestTcpDiscoverySpi discoSpi = (TestTcpDiscoverySpi)cfg.getDiscoverySpi();
- discoSpi.setIpFinder(IP_FINDER);
+ DiscoveryHook discoveryHook = new DiscoveryHook() {
+ @Override public void beforeDiscovery(DiscoverySpiCustomMessage msg) {
+ DiscoveryCustomMessage customMsg = msg == null ?
Review comment:
I think we should provide `customMsg` as argument (or instead of `DiscoverySpiCustomMessage msg`) and do not duplicate:
```
DiscoveryCustomMessage customMsg = msg == null ?
null : (DiscoveryCustomMessage)IgniteUtils.field(msg, "delegate");
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov merged pull request #7647: IGNITE-12879 Refactors
test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
nizhikov merged pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409389076
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/binary/GridCacheBinaryObjectMetadataExchangeMultinodeTest.java
##########
@@ -129,13 +115,10 @@ public void testReadRequestBlockedOnUpdatingMetadata() throws Exception {
discoveryHook = new DiscoveryHook() {
private volatile IgniteEx ignite;
- @Override public void handleDiscoveryMessage(DiscoverySpiCustomMessage msg) {
+ @Override public void beforeDiscovery(DiscoveryCustomMessage customMsg) {
if (finishFut.isDone())
return;
- DiscoveryCustomMessage customMsg = msg == null ? null
Review comment:
Why this deletion?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409423030
##########
File path: modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TestTcpDiscoverySpi.java
##########
@@ -44,4 +54,24 @@
@Override public void simulateNodeFailure() {
super.simulateNodeFailure();
}
+
+ /** {@inheritDoc} */
+ @SuppressWarnings("ZeroLengthArrayAllocation")
+ @Override public void setListener(@Nullable DiscoverySpiListener lsnr) {
+ super.setListener(lsnr == null || discoHooks.isEmpty() ?
Review comment:
OK then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] NSAmelchev commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r406285358
##########
File path: modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TestTcpDiscoverySpi.java
##########
@@ -44,4 +52,19 @@
@Override public void simulateNodeFailure() {
super.simulateNodeFailure();
}
+
+ /** {@inheritDoc} */
+ @Override public void setListener(@Nullable DiscoverySpiListener lsnr) {
+ super.setListener(lsnr == null || discoveryHooks == null ? lsnr : wrap(lsnr, discoveryHooks));
+ }
+
+ /**
+ * Sets interceptors of discovery messages.
+ *
+ * @param discoveryHooks Interceptors of discovery messages.
+ */
+ @SuppressWarnings("AssignmentOrReturnOfFieldWithMutableType")
+ public void discoveryHooks(DiscoveryHook... discoveryHooks) {
Review comment:
Why array needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] NSAmelchev commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r406296781
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/continuous/IgniteContinuousQueryMetadataUpdateTest.java
##########
@@ -57,11 +55,26 @@
@Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
- TestDiscoverySpi discoSpi = new TestDiscoverySpi();
Review comment:
Why this changed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409388734
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/binary/BinaryMetadataUpdatesFlowTest.java
##########
@@ -209,10 +197,7 @@ public void testFlowNoConflictsWithClients() throws Exception {
return;
discoveryHook = new DiscoveryHook() {
- @Override public void handleDiscoveryMessage(DiscoverySpiCustomMessage msg) {
- DiscoveryCustomMessage customMsg = msg == null ? null
Review comment:
Why this deletion?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ololo3000 commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409393228
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/binary/BinaryMetadataUpdatesFlowTest.java
##########
@@ -209,10 +197,7 @@ public void testFlowNoConflictsWithClients() throws Exception {
return;
discoveryHook = new DiscoveryHook() {
- @Override public void handleDiscoveryMessage(DiscoverySpiCustomMessage msg) {
- DiscoveryCustomMessage customMsg = msg == null ? null
Review comment:
This logic was moved to DiscoveryHook class to avoid duplication.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ololo3000 commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409426767
##########
File path: modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TestTcpDiscoverySpi.java
##########
@@ -31,6 +38,9 @@
/** */
public boolean ignorePingResponse;
+ /** Interceptors of discovery messages. */
+ private List<DiscoveryHook> discoHooks = new ArrayList<>();
Review comment:
Thanks. Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409396225
##########
File path: modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TestTcpDiscoverySpi.java
##########
@@ -44,4 +54,24 @@
@Override public void simulateNodeFailure() {
super.simulateNodeFailure();
}
+
+ /** {@inheritDoc} */
+ @SuppressWarnings("ZeroLengthArrayAllocation")
+ @Override public void setListener(@Nullable DiscoverySpiListener lsnr) {
+ super.setListener(lsnr == null || discoHooks.isEmpty() ?
Review comment:
It seems to me that we can just pass discoHooks into the listener.
Therefore list always be up to date inside the listener.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409394624
##########
File path: modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TestTcpDiscoverySpi.java
##########
@@ -31,6 +38,9 @@
/** */
public boolean ignorePingResponse;
+ /** Interceptors of discovery messages. */
+ private List<DiscoveryHook> discoHooks = new ArrayList<>();
Review comment:
Let's allocate discoHooks on the first hook addition.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] NSAmelchev commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r406295006
##########
File path: modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TestTcpDiscoverySpi.java
##########
@@ -44,4 +52,19 @@
@Override public void simulateNodeFailure() {
super.simulateNodeFailure();
}
+
+ /** {@inheritDoc} */
+ @Override public void setListener(@Nullable DiscoverySpiListener lsnr) {
+ super.setListener(lsnr == null || discoveryHooks == null ? lsnr : wrap(lsnr, discoveryHooks));
+ }
+
+ /**
+ * Sets interceptors of discovery messages.
Review comment:
I think we should add javadoc and assert that hook should be set before Discovery started.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] NSAmelchev commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
NSAmelchev commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r406296781
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/continuous/IgniteContinuousQueryMetadataUpdateTest.java
##########
@@ -57,11 +55,26 @@
@Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
- TestDiscoverySpi discoSpi = new TestDiscoverySpi();
Review comment:
Why this changed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] nizhikov commented on a change in pull request #7647:
IGNITE-12879 Refactors test configuration of discovery messages interceptors.
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #7647: IGNITE-12879 Refactors test configuration of discovery messages interceptors.
URL: https://github.com/apache/ignite/pull/7647#discussion_r409393898
##########
File path: modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TestTcpDiscoverySpi.java
##########
@@ -44,4 +54,24 @@
@Override public void simulateNodeFailure() {
super.simulateNodeFailure();
}
+
+ /** {@inheritDoc} */
+ @SuppressWarnings("ZeroLengthArrayAllocation")
+ @Override public void setListener(@Nullable DiscoverySpiListener lsnr) {
+ super.setListener(lsnr == null || discoHooks.isEmpty() ?
Review comment:
Is the following flow possible? If no, why?
1. addDiscoveryHook
2. setListener
3. addDiscoveryHook <- this hook will be ignored.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services