You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/09/22 19:57:54 UTC
[GitHub] [geode] jdeppe-pivotal opened a new pull request #6891: GEODE-9627: Add service loader interface to register DataSerializableFixedIDs
jdeppe-pivotal opened a new pull request #6891:
URL: https://github.com/apache/geode/pull/6891
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r727383439
##########
File path: geode-wan/src/main/java/org/apache/geode/cache/wan/internal/client/locator/WANFactoryImpl.java
##########
@@ -27,37 +27,16 @@
import org.apache.geode.cache.wan.internal.GatewayReceiverFactoryImpl;
import org.apache.geode.cache.wan.internal.GatewaySenderFactoryImpl;
import org.apache.geode.distributed.internal.WanLocatorDiscoverer;
-import org.apache.geode.internal.InternalDataSerializer;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.wan.spi.WANFactory;
import org.apache.geode.internal.serialization.DataSerializableFixedID;
+import org.apache.geode.internal.serialization.DataSerializableFixedIdLoader;
+import org.apache.geode.internal.serialization.DataSerializableFixedIdRegistrar;
-public class WANFactoryImpl implements WANFactory {
+public class WANFactoryImpl implements WANFactory, DataSerializableFixedIdLoader {
@Override
- public void initialize() {
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_JOIN_REQUEST,
- RemoteLocatorJoinRequest.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_JOIN_RESPONSE,
- RemoteLocatorJoinResponse.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_REQUEST,
- RemoteLocatorRequest.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.LOCATOR_JOIN_MESSAGE,
- LocatorJoinMessage.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_PING_REQUEST,
- RemoteLocatorPingRequest.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_PING_RESPONSE,
- RemoteLocatorPingResponse.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_RESPONSE,
- RemoteLocatorResponse.class);
- }
+ public void initialize() {}
Review comment:
i like Kirk's idea
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6891:
URL: https://github.com/apache/geode/pull/6891#issuecomment-938159824
Force push due to large redis rename commit
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #6891:
URL: https://github.com/apache/geode/pull/6891#issuecomment-938159824
Force push due to large redis rename commit
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r723451576
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/BackwardCompatibilitySerializationDUnitTest.java
##########
@@ -68,21 +67,19 @@ public BackwardCompatibilitySerializationDUnitTest() {
super();
}
+ static {
+ InternalDataSerializer.getDSFIDSerializer()
+ .register(TestMessage.TEST_MESSAGE_DSFID, TestMessage.class);
+ }
+
Review comment:
Good call. Somehow I thought `@BeforeClass` wouldn't work in something that extends `JUnit4*TestCase`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal merged pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #6891:
URL: https://github.com/apache/geode/pull/6891
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r723448213
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java
##########
@@ -246,28 +255,19 @@ public void createEntryWithBigMessage() {
@Test
public void receiveBigResponse() {
- invokeInEveryVM(
- () -> InternalDataSerializer.getDSFIDSerializer().registerDSFID(SERIAL_ACKED_MESSAGE,
- SerialAckedMessageWithBigReply.class));
- try {
- int locatorPort = createLocator(getVM(0));
- for (int i = 1; i <= NUM_SERVERS; i++) {
- createCacheAndRegion(getVM(i), locatorPort);
- }
- DistributedMember vm2ID =
- getVM(2).invoke(() -> cache.getDistributedSystem().getDistributedMember());
- getVM(1).invoke("receive a large direct-reply message", () -> {
- SerialAckedMessageWithBigReply messageWithBigReply = new SerialAckedMessageWithBigReply();
- await().until(() -> {
- messageWithBigReply.send(Collections.singleton(vm2ID));
- return true;
- });
- });
- } finally {
- invokeInEveryVM(
- () -> InternalDataSerializer.getDSFIDSerializer().registerDSFID(SERIAL_ACKED_MESSAGE,
- SerialAckedMessage.class));
+ int locatorPort = createLocator(getVM(0));
+ for (int i = 1; i <= NUM_SERVERS; i++) {
+ createCacheAndRegion(getVM(i), locatorPort);
}
+ DistributedMember vm2ID =
+ getVM(2).invoke(() -> cache.getDistributedSystem().getDistributedMember());
+ getVM(1).invoke("receive a large direct-reply message", () -> {
+ SerialAckedMessageWithBigReply messageWithBigReply = new SerialAckedMessageWithBigReply();
+ await().until(() -> {
+ messageWithBigReply.send(Collections.singleton(vm2ID));
+ return true;
+ });
Review comment:
It looks like it will catch a hang in the `send` call. If I break this test by not registering any fixed Ids, then the remote end (of the `send`) throws an exception but no reply ever comes back. This seems like a much bigger issue...
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r727470439
##########
File path: geode-for-redis/src/main/resources/META-INF/services/org.apache.geode.internal.DSFIDLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.redis.internal.GeodeRedisService
Review comment:
Thanks for catching that - these crept back in when I rebased and should be deleted.
##########
File path: geode-for-redis/src/main/resources/META-INF/services/org.apache.geode.internal.DSFIDLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.redis.internal.GeodeRedisService
Review comment:
Thanks for catching that - these crept back in when I rebased and will be deleted.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r727422196
##########
File path: geode-for-redis/src/main/resources/META-INF/services/org.apache.geode.internal.DSFIDLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.redis.internal.GeodeRedisService
Review comment:
This file doesn't have the Apache license header, (neither do the equivalent files for `DSFIDLoader` and `DataSerializableFixedIdLoader`) but the one for `DataSerializableFixedIdRegistrant` does. For consistency, should all these files either include or not include the header?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r723451967
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/internal/DSFIDSerializerImpl.java
##########
@@ -336,7 +336,7 @@ public void invokeFromData(Object ds, DataInput in)
@Override
- public void registerDSFID(int dsfid, Class<?> dsfidClass) {
+ public void register(int dsfid, Class<? extends DataSerializableFixedID> dsfidClass) {
Review comment:
Added
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r723603037
##########
File path: geode-apis-compatible-with-redis/src/main/resources/META-INF/services/org.apache.geode.internal.serialization.DataSerializableFixedIdLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.redis.internal.GeodeRedisService
Review comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r727422196
##########
File path: geode-for-redis/src/main/resources/META-INF/services/org.apache.geode.internal.DSFIDLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.redis.internal.GeodeRedisService
Review comment:
This file doesn't have the Apache license header, (neither do the equivalent files for `DSFIDLoader` and `DataSerializableFixedIdLoader`) but the one for `DataSerializableFixedIdRegistrant` does. For consistency, should all these files either include or not include the header?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r723442699
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedIdRegistrar.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.serialization;
+
+public interface DataSerializableFixedIdRegistrar {
+
+ /**
+ * Register the constructor for a fixed ID class. Use this to register your
+ * DataSerializableFixedID classes so that deserialization knows how to instantiate them.
+ */
+ void register(int dsfid, Class<? extends DataSerializableFixedID> dsfidClass);
Review comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r723449425
##########
File path: geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java
##########
@@ -329,6 +331,17 @@ public void invokeFromData(Object ds, DataInput in)
}).create();
initializeWellKnownSerializers();
dsfidFactory = new DSFIDFactory(dsfidSerializer);
+
+ ServiceLoader<DataSerializableFixedIdLoader> loaders = ServiceLoader.load(
+ DataSerializableFixedIdLoader.class);
+ for (DataSerializableFixedIdLoader loader : loaders) {
+ try {
+ loader.register(dsfidSerializer);
+ } catch (Exception ex) {
+ logger.warn("Data serializable fixed ID loader '" + loader.getClass().getName() +
+ "' failed", ex);
Review comment:
Which is probably what I did... :( Fixed now.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/BackwardCompatibilitySerializationDUnitTest.java
##########
@@ -249,6 +248,8 @@ private void resetFlags() {
private static final KnownVersion[] dsfidVersions =
new KnownVersion[] {KnownVersion.GEODE_1_1_0, KnownVersion.GEODE_1_5_0};
+ static final int TEST_MESSAGE_DSFID = 12345;
+
Review comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r723442923
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedIdRegistrar.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.serialization;
+
+public interface DataSerializableFixedIdRegistrar {
Review comment:
Done
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedIdLoader.java
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.serialization;
+
+
+/**
+ * Service loader interface used to register Data Serializable Fixed IDs. Geode
+ * {@code CacheService}s should implement this SPI instead of explicitly registering DSFIDs during
+ * service initialization.
+ */
+public interface DataSerializableFixedIdLoader {
Review comment:
Done
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedIdLoader.java
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.serialization;
+
+
+/**
+ * Service loader interface used to register Data Serializable Fixed IDs. Geode
+ * {@code CacheService}s should implement this SPI instead of explicitly registering DSFIDs during
+ * service initialization.
+ */
+public interface DataSerializableFixedIdLoader {
+
+ void register(DataSerializableFixedIdRegistrar serializer);
Review comment:
Done
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DSFIDSerializer.java
##########
@@ -27,7 +27,7 @@
* <p>
* Use DSFIDSerializerFactory to construct a serialization service.
*/
-public interface DSFIDSerializer {
+public interface DSFIDSerializer extends DataSerializableFixedIdRegistrar {
Review comment:
Done
##########
File path: geode-lucene/src/main/resources/META-INF/services/org.apache.geode.internal.serialization.DataSerializableFixedIdLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.cache.lucene.internal.LuceneServiceImpl
Review comment:
Fixed
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r723297754
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java
##########
@@ -138,6 +138,15 @@
@Rule
public SerializableTestName testName = new SerializableTestName();
+ @BeforeClass
+ public static void setup() {
+ DUnitLauncher.launchIfNeeded(NUM_SERVERS + 1);
+ invokeInEveryVM(
+ () -> InternalDataSerializer.getDSFIDSerializer().register(
+ SerialAckedMessageWithBigReply.DSFID,
+ SerialAckedMessageWithBigReply.class));
+ }
+
Review comment:
Since I've added checks to throw an exception on duplicate fixed Id registration, this can only happen once - hence putting it in a `@BeforeClass`.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r727470439
##########
File path: geode-for-redis/src/main/resources/META-INF/services/org.apache.geode.internal.DSFIDLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.redis.internal.GeodeRedisService
Review comment:
Thanks for catching that - these crept back in when I rebased and should be deleted.
##########
File path: geode-for-redis/src/main/resources/META-INF/services/org.apache.geode.internal.DSFIDLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.redis.internal.GeodeRedisService
Review comment:
Thanks for catching that - these crept back in when I rebased and will be deleted.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r722639175
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedIdLoader.java
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.serialization;
+
+
+/**
+ * Service loader interface used to register Data Serializable Fixed IDs. Geode
+ * {@code CacheService}s should implement this SPI instead of explicitly registering DSFIDs during
+ * service initialization.
+ */
+public interface DataSerializableFixedIdLoader {
+
+ void register(DataSerializableFixedIdRegistrar serializer);
Review comment:
I think you should rename the parameter to `registrar` here and in all implementations. If you use IntelliJ "Refactor > Change Signature", it'll rename the parameter in all of the implementations as well.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r722636400
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedIdRegistrar.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.serialization;
+
+public interface DataSerializableFixedIdRegistrar {
+
+ /**
+ * Register the constructor for a fixed ID class. Use this to register your
+ * DataSerializableFixedID classes so that deserialization knows how to instantiate them.
+ */
+ void register(int dsfid, Class<? extends DataSerializableFixedID> dsfidClass);
Review comment:
How naming the parameters `fixedId` and `fixedIdClass` to avoid abbreviations?
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedIdLoader.java
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.serialization;
+
+
+/**
+ * Service loader interface used to register Data Serializable Fixed IDs. Geode
+ * {@code CacheService}s should implement this SPI instead of explicitly registering DSFIDs during
+ * service initialization.
+ */
+public interface DataSerializableFixedIdLoader {
+
+ void register(DataSerializableFixedIdRegistrar serializer);
Review comment:
I think you should rename the parameter to `registrar` here and in all implementations. If you use IntelliJ "Refactor > Change Signature", it'll rename the parameter in all of the implementation as well.
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/internal/DSFIDSerializerImpl.java
##########
@@ -336,7 +336,7 @@ public void invokeFromData(Object ds, DataInput in)
@Override
- public void registerDSFID(int dsfid, Class<?> dsfidClass) {
+ public void register(int dsfid, Class<? extends DataSerializableFixedID> dsfidClass) {
Review comment:
I think you should create a new unit test DSFIDSerializerImplTest. Just add test(s) for this method and focus on covering the if-else block where you added the most code.
##########
File path: geode-lucene/src/main/resources/META-INF/services/org.apache.geode.internal.serialization.DataSerializableFixedIdLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.cache.lucene.internal.LuceneServiceImpl
Review comment:
This service file should have an Apache license header.
##########
File path: geode-wan/src/main/java/org/apache/geode/cache/wan/internal/client/locator/WANFactoryImpl.java
##########
@@ -27,37 +27,16 @@
import org.apache.geode.cache.wan.internal.GatewayReceiverFactoryImpl;
import org.apache.geode.cache.wan.internal.GatewaySenderFactoryImpl;
import org.apache.geode.distributed.internal.WanLocatorDiscoverer;
-import org.apache.geode.internal.InternalDataSerializer;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.wan.spi.WANFactory;
import org.apache.geode.internal.serialization.DataSerializableFixedID;
+import org.apache.geode.internal.serialization.DataSerializableFixedIdLoader;
+import org.apache.geode.internal.serialization.DataSerializableFixedIdRegistrar;
-public class WANFactoryImpl implements WANFactory {
+public class WANFactoryImpl implements WANFactory, DataSerializableFixedIdLoader {
@Override
- public void initialize() {
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_JOIN_REQUEST,
- RemoteLocatorJoinRequest.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_JOIN_RESPONSE,
- RemoteLocatorJoinResponse.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_REQUEST,
- RemoteLocatorRequest.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.LOCATOR_JOIN_MESSAGE,
- LocatorJoinMessage.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_PING_REQUEST,
- RemoteLocatorPingRequest.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_PING_RESPONSE,
- RemoteLocatorPingResponse.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_RESPONSE,
- RemoteLocatorResponse.class);
- }
+ public void initialize() {}
Review comment:
[comment] You might consider giving `initialize` an empty default in the interface.
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedIdLoader.java
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.serialization;
+
+
+/**
+ * Service loader interface used to register Data Serializable Fixed IDs. Geode
+ * {@code CacheService}s should implement this SPI instead of explicitly registering DSFIDs during
+ * service initialization.
+ */
+public interface DataSerializableFixedIdLoader {
Review comment:
You should annotate this with `@FunctionalInterface`.
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DSFIDSerializer.java
##########
@@ -27,7 +27,7 @@
* <p>
* Use DSFIDSerializerFactory to construct a serialization service.
*/
-public interface DSFIDSerializer {
+public interface DSFIDSerializer extends DataSerializableFixedIdRegistrar {
Review comment:
Now that this interface only has one method defined, you should annotate it with `@FunctionalInterface`.
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java
##########
@@ -138,6 +138,15 @@
@Rule
public SerializableTestName testName = new SerializableTestName();
+ @BeforeClass
+ public static void setup() {
+ DUnitLauncher.launchIfNeeded(NUM_SERVERS + 1);
+ invokeInEveryVM(
+ () -> InternalDataSerializer.getDSFIDSerializer().register(
+ SerialAckedMessageWithBigReply.DSFID,
+ SerialAckedMessageWithBigReply.class));
+ }
+
Review comment:
This is redundant with the `DistributedRule`. I would change this and the rule on line 131 to:
```
@Rule
public DistributedRule distributedRule = new DistributedRule(NUM_SERVERS + 1);
@Before
public void setUp() {
invokeInEveryVM(() -> InternalDataSerializer.getDSFIDSerializer().register(
SerialAckedMessageWithBigReply.DSFID,
SerialAckedMessageWithBigReply.class));
}
```
##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedIdRegistrar.java
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.internal.serialization;
+
+public interface DataSerializableFixedIdRegistrar {
Review comment:
You should annotate this with `@FunctionalInterface`.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/BackwardCompatibilitySerializationDUnitTest.java
##########
@@ -249,6 +248,8 @@ private void resetFlags() {
private static final KnownVersion[] dsfidVersions =
new KnownVersion[] {KnownVersion.GEODE_1_1_0, KnownVersion.GEODE_1_5_0};
+ static final int TEST_MESSAGE_DSFID = 12345;
+
Review comment:
I would make this `private` unless another class in this package needs to reference it.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/BackwardCompatibilitySerializationDUnitTest.java
##########
@@ -68,21 +67,19 @@ public BackwardCompatibilitySerializationDUnitTest() {
super();
}
+ static {
+ InternalDataSerializer.getDSFIDSerializer()
+ .register(TestMessage.TEST_MESSAGE_DSFID, TestMessage.class);
+ }
+
Review comment:
Since this is a test, I recommend moving it to `postSetUp`, a new `@Before` method, or a new `@BeforeClass` method.
I think we should avoid static initializers when possible -- if the code fails, the class fails to load and a NoClassDefFoundError is thrown which is hard to debug.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java
##########
@@ -329,6 +331,17 @@ public void invokeFromData(Object ds, DataInput in)
}).create();
initializeWellKnownSerializers();
dsfidFactory = new DSFIDFactory(dsfidSerializer);
+
+ ServiceLoader<DataSerializableFixedIdLoader> loaders = ServiceLoader.load(
+ DataSerializableFixedIdLoader.class);
+ for (DataSerializableFixedIdLoader loader : loaders) {
+ try {
+ loader.register(dsfidSerializer);
+ } catch (Exception ex) {
+ logger.warn("Data serializable fixed ID loader '" + loader.getClass().getName() +
+ "' failed", ex);
Review comment:
Log4j2 preferred syntax is:
```
logger.warn("Data serializable fixed ID loader '{}' failed", loader.getClass().getName(), ex);
```
I would change it mostly to avoid having others copy the concatenation syntax.
##########
File path: geode-apis-compatible-with-redis/src/main/resources/META-INF/services/org.apache.geode.internal.serialization.DataSerializableFixedIdLoader
##########
@@ -0,0 +1 @@
+org.apache.geode.redis.internal.GeodeRedisService
Review comment:
This service file should have an Apache license header.
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java
##########
@@ -246,28 +255,19 @@ public void createEntryWithBigMessage() {
@Test
public void receiveBigResponse() {
- invokeInEveryVM(
- () -> InternalDataSerializer.getDSFIDSerializer().registerDSFID(SERIAL_ACKED_MESSAGE,
- SerialAckedMessageWithBigReply.class));
- try {
- int locatorPort = createLocator(getVM(0));
- for (int i = 1; i <= NUM_SERVERS; i++) {
- createCacheAndRegion(getVM(i), locatorPort);
- }
- DistributedMember vm2ID =
- getVM(2).invoke(() -> cache.getDistributedSystem().getDistributedMember());
- getVM(1).invoke("receive a large direct-reply message", () -> {
- SerialAckedMessageWithBigReply messageWithBigReply = new SerialAckedMessageWithBigReply();
- await().until(() -> {
- messageWithBigReply.send(Collections.singleton(vm2ID));
- return true;
- });
- });
- } finally {
- invokeInEveryVM(
- () -> InternalDataSerializer.getDSFIDSerializer().registerDSFID(SERIAL_ACKED_MESSAGE,
- SerialAckedMessage.class));
+ int locatorPort = createLocator(getVM(0));
+ for (int i = 1; i <= NUM_SERVERS; i++) {
+ createCacheAndRegion(getVM(i), locatorPort);
}
+ DistributedMember vm2ID =
+ getVM(2).invoke(() -> cache.getDistributedSystem().getDistributedMember());
+ getVM(1).invoke("receive a large direct-reply message", () -> {
+ SerialAckedMessageWithBigReply messageWithBigReply = new SerialAckedMessageWithBigReply();
+ await().until(() -> {
+ messageWithBigReply.send(Collections.singleton(vm2ID));
+ return true;
+ });
Review comment:
Looks like this code was pre-existing and just moved, but it stands out as a very weird usage of `await`. Does it need to be in an `await`? Seems like it should either be a simple call without `await` or a `Mockito.verify` done on a spy of `messageWithBigReply`.
If the `await` is supposed to retry the `send` multiple times, then what causes the `send` to not work the first time? Does it throw? If it throws, it would need some form of `ignoreException` clause added to it.
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java
##########
@@ -246,28 +255,19 @@ public void createEntryWithBigMessage() {
@Test
public void receiveBigResponse() {
- invokeInEveryVM(
- () -> InternalDataSerializer.getDSFIDSerializer().registerDSFID(SERIAL_ACKED_MESSAGE,
- SerialAckedMessageWithBigReply.class));
- try {
- int locatorPort = createLocator(getVM(0));
- for (int i = 1; i <= NUM_SERVERS; i++) {
- createCacheAndRegion(getVM(i), locatorPort);
- }
- DistributedMember vm2ID =
- getVM(2).invoke(() -> cache.getDistributedSystem().getDistributedMember());
- getVM(1).invoke("receive a large direct-reply message", () -> {
- SerialAckedMessageWithBigReply messageWithBigReply = new SerialAckedMessageWithBigReply();
- await().until(() -> {
- messageWithBigReply.send(Collections.singleton(vm2ID));
- return true;
- });
- });
- } finally {
- invokeInEveryVM(
- () -> InternalDataSerializer.getDSFIDSerializer().registerDSFID(SERIAL_ACKED_MESSAGE,
- SerialAckedMessage.class));
+ int locatorPort = createLocator(getVM(0));
+ for (int i = 1; i <= NUM_SERVERS; i++) {
+ createCacheAndRegion(getVM(i), locatorPort);
}
+ DistributedMember vm2ID =
Review comment:
[comment] How about `memberVM2` or `memberInVM2` instead of `vm2ID`?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r727383439
##########
File path: geode-wan/src/main/java/org/apache/geode/cache/wan/internal/client/locator/WANFactoryImpl.java
##########
@@ -27,37 +27,16 @@
import org.apache.geode.cache.wan.internal.GatewayReceiverFactoryImpl;
import org.apache.geode.cache.wan.internal.GatewaySenderFactoryImpl;
import org.apache.geode.distributed.internal.WanLocatorDiscoverer;
-import org.apache.geode.internal.InternalDataSerializer;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.wan.spi.WANFactory;
import org.apache.geode.internal.serialization.DataSerializableFixedID;
+import org.apache.geode.internal.serialization.DataSerializableFixedIdLoader;
+import org.apache.geode.internal.serialization.DataSerializableFixedIdRegistrar;
-public class WANFactoryImpl implements WANFactory {
+public class WANFactoryImpl implements WANFactory, DataSerializableFixedIdLoader {
@Override
- public void initialize() {
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_JOIN_REQUEST,
- RemoteLocatorJoinRequest.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_JOIN_RESPONSE,
- RemoteLocatorJoinResponse.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_REQUEST,
- RemoteLocatorRequest.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.LOCATOR_JOIN_MESSAGE,
- LocatorJoinMessage.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_PING_REQUEST,
- RemoteLocatorPingRequest.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_PING_RESPONSE,
- RemoteLocatorPingResponse.class);
- InternalDataSerializer.getDSFIDSerializer().registerDSFID(
- DataSerializableFixedID.REMOTE_LOCATOR_RESPONSE,
- RemoteLocatorResponse.class);
- }
+ public void initialize() {}
Review comment:
i like Kirk's idea
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6891: GEODE-9627: Add service provider interface to register DataSerializableFixedIDs
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6891:
URL: https://github.com/apache/geode/pull/6891#discussion_r723448213
##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/ClusterCommunicationsDUnitTest.java
##########
@@ -246,28 +255,19 @@ public void createEntryWithBigMessage() {
@Test
public void receiveBigResponse() {
- invokeInEveryVM(
- () -> InternalDataSerializer.getDSFIDSerializer().registerDSFID(SERIAL_ACKED_MESSAGE,
- SerialAckedMessageWithBigReply.class));
- try {
- int locatorPort = createLocator(getVM(0));
- for (int i = 1; i <= NUM_SERVERS; i++) {
- createCacheAndRegion(getVM(i), locatorPort);
- }
- DistributedMember vm2ID =
- getVM(2).invoke(() -> cache.getDistributedSystem().getDistributedMember());
- getVM(1).invoke("receive a large direct-reply message", () -> {
- SerialAckedMessageWithBigReply messageWithBigReply = new SerialAckedMessageWithBigReply();
- await().until(() -> {
- messageWithBigReply.send(Collections.singleton(vm2ID));
- return true;
- });
- });
- } finally {
- invokeInEveryVM(
- () -> InternalDataSerializer.getDSFIDSerializer().registerDSFID(SERIAL_ACKED_MESSAGE,
- SerialAckedMessage.class));
+ int locatorPort = createLocator(getVM(0));
+ for (int i = 1; i <= NUM_SERVERS; i++) {
+ createCacheAndRegion(getVM(i), locatorPort);
}
+ DistributedMember vm2ID =
+ getVM(2).invoke(() -> cache.getDistributedSystem().getDistributedMember());
+ getVM(1).invoke("receive a large direct-reply message", () -> {
+ SerialAckedMessageWithBigReply messageWithBigReply = new SerialAckedMessageWithBigReply();
+ await().until(() -> {
+ messageWithBigReply.send(Collections.singleton(vm2ID));
+ return true;
+ });
Review comment:
It looks like the `await` will catch a hang in the `send` call. If I break this test by not registering any fixed Ids, then the remote end (of the `send`) throws an exception but no reply ever comes back. This seems like a much bigger issue...
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org