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 2022/02/05 04:37:11 UTC
[GitHub] [geode] ezoerner opened a new pull request #7340: GEODE-9998 upgrade Jedis client to 4.1.1
ezoerner opened a new pull request #7340:
URL: https://github.com/apache/geode/pull/7340
<!-- 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] DonalEvans commented on a change in pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r809235663
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/pubsub/AbstractSubscriptionsIntegrationTest.java
##########
@@ -74,9 +74,6 @@ public void pingWithArgumentWhileSubscribed() {
executor.submit(() -> client.subscribe(mockSubscriber, "same"));
mockSubscriber.awaitSubscribe("same");
mockSubscriber.ping("potato");
- // JedisPubSub PING with message is not currently possible, will submit a PR
- // (https://github.com/xetorthio/jedis/issues/2049)
- // until then, we have to call this second ping to flush the client
mockSubscriber.ping();
Review comment:
This second ping should also be removed, as it's no longer necessary, and the assertion on line 79 should be changed from `isEqualTo(2)` to `isOne()`
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -435,30 +436,30 @@ public void shouldUnionize_givenSetsPartiallyOverlap() {
Map<String, Double> scores2 = makeScoreMap(5, 1, 10, x -> (double) (x < 10 ? x : x * 2));
jedis.zadd(KEY2, scores2);
- Set<Tuple> expectedResults = convertToTuples(makeScoreMap(0, 1, 15, x -> (double) x),
+ final List<Tuple> expectedResults = convertToTuples(makeScoreMap(0, 1, 15, x -> (double) x),
(i, x) -> (double) (i < 5 ? i : i * 2));
jedis.zunionstore(NEW_SET, KEY1, KEY2);
- Set<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, 20);
+ final List<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, 20);
assertThat(results).containsExactlyElementsOf(expectedResults);
}
@Test
- public void ensureWeightsAreAppliedBeforeAggregation() {
+ public void ensureWeightsAreAppliedBeforeAxggregation() {
Review comment:
Typo here, "Axggregation" should be "Aggregation"
--
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] onichols-pivotal commented on pull request #7340: GEODE-9998 upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #7340:
URL: https://github.com/apache/geode/pull/7340#issuecomment-1037048470
> > First commit (and also PR title) must begin with `GEODE-nnnnn: `. Please amend your initial commit and force-push.
>
> @onichols-pivotal I think this message might have been automatically generated in error, since the JIRA ticket number is GEODE-9998. Adding a zero to the front of the number is just likely to cause confusion, so I think this should probably be put back as it was.
Sorry if that was unclear. Not asking anything to be zero-padded! that would make no sense at all! Just asking that the ticket number be included at the beginning of the commit message FOLLOWED BY <COLON><SPACE>. Looks like you were just missing the colon.
--
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 #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r806047523
##########
File path: geode-for-redis/src/commonTest/java/org/apache/geode/redis/mocks/MockSubscriber.java
##########
@@ -45,8 +43,9 @@
Collections.synchronizedList(new ArrayList<>());
private CountDownLatch messageReceivedLatch = new CountDownLatch(0);
private CountDownLatch pMessageReceivedLatch = new CountDownLatch(0);
- private String localSocketAddress;
- private Client client;
+ /*
+ * private String localSocketAddress;
+ */
Review comment:
Probably delete this.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -234,20 +235,20 @@ public void shouldUnionize_givenBoundaryScoresAndWeights() {
jedis.zunionstore(NEW_SET, new ZParams().weights(1), KEY1);
- Set<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, scores.size());
- assertThat(results).containsExactlyElementsOf(expectedResults);
+ List<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, scores.size());
+ assertThat(results).containsExactlyInAnyOrderElementsOf(expectedResults);
Review comment:
Please keep all of these assertions as `containsExactlyElementsOf` if possible (I ran a quick test and looks like it's OK). Since `ZRANGESCORE` is supposed to return an ordered list, we should check that.
##########
File path: geode-for-redis/src/commonTest/java/org/apache/geode/redis/mocks/MockSubscriber.java
##########
@@ -65,12 +64,24 @@ public MockSubscriber(CountDownLatch subscriptionLatch, CountDownLatch unsubscri
this.pUnsubscriptionLatch = pUnsubscriptionLatch;
}
- @Override
- public void proceed(Client client, String... channels) {
- localSocketAddress = client.getSocket().getLocalSocketAddress().toString();
- this.client = client;
- super.proceed(client, channels);
- }
+ /*
+ * @Override
+ * public void proceed(final Connection connection, final String... channels) {
+ * try {
+ * // Kludge due to socket becoming private in jedis 4.1.1
+ * // TODO is there a safe public way of getting local socket address
+ * // This doesn't work (or no longer works in Jedis 4.1.1), results in null socket
+ * final Field privateSocketField = Connection.class.getDeclaredField("socket");
+ * privateSocketField.setAccessible(true);
+ * final Socket socket = (Socket) privateSocketField.get(connection);
+ *
+ * localSocketAddress = socket.getLocalSocketAddress().toString();
+ * } catch (final NoSuchFieldException | IllegalAccessException ex) {
+ * throw new RuntimeException("Error in accessing private field 'socket' via reflection", ex);
+ * }
+ * super.proceed(connection, channels);
+ * }
+ */
Review comment:
Also 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] onichols-pivotal edited a comment on pull request #7340: GEODE-9998 upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
onichols-pivotal edited a comment on pull request #7340:
URL: https://github.com/apache/geode/pull/7340#issuecomment-1037048470
> > First commit (and also PR title) must begin with `GEODE-nnnnn: `. Please amend your initial commit and force-push.
>
> @onichols-pivotal I think this message might have been automatically generated in error, since the JIRA ticket number is GEODE-9998. Adding a zero to the front of the number is just likely to cause confusion, so I think this should probably be put back as it was.
Sorry if that was unclear. Not asking anything to be zero-padded! that would make no sense at all! Just asking that the ticket number be included at the beginning of the commit message FOLLOWED BY {COLON}{SPACE}. Looks like you were just missing the colon.
--
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] onichols-pivotal edited a comment on pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
onichols-pivotal edited a comment on pull request #7340:
URL: https://github.com/apache/geode/pull/7340#issuecomment-1037048470
> > First commit (and also PR title) must begin with `GEODE-nnnnn: `. Please amend your initial commit and force-push.
>
> @onichols-pivotal I think this message might have been automatically generated in error, since the JIRA ticket number is GEODE-9998. Adding a zero to the front of the number is just likely to cause confusion, so I think this should probably be put back as it was.
Sorry if that was unclear. Not asking anything to be zero-padded! that would make no sense at all! Just asking that the ticket number be included at the beginning of the commit message FOLLOWED BY {COLON}{SPACE}. Looks like you were just missing the colon.
--
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] ezoerner commented on a change in pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
ezoerner commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r808180787
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
##########
@@ -216,11 +216,22 @@ public void shouldReturnRange_givenLeftParenOnlyForMinOrMax() {
public void shouldReturnRange_boundedByLimit() {
createZSetRangeTestMap();
- assertThat(jedis.zrevrangeByScore(KEY, "10", "0", 0, 2))
+ /*
+ * Fails in Jedis 4.1.1 for both Geode and Native Redis, so must be a Jedis bug
+ * TODO submit GitHub issue for this
Review comment:
It's not clear to me that the localSocketAddress is actually necessary here. I think this may have been part of the thread name for debugging purposes or for trivial reasons. So I will delete the commented out code, but only file a bug if someone thinks it is necessary. The test works fine without it.
--
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] ezoerner commented on a change in pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
ezoerner commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r808597239
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
##########
@@ -216,11 +216,22 @@ public void shouldReturnRange_givenLeftParenOnlyForMinOrMax() {
public void shouldReturnRange_boundedByLimit() {
createZSetRangeTestMap();
- assertThat(jedis.zrevrangeByScore(KEY, "10", "0", 0, 2))
+ /*
+ * Fails in Jedis 4.1.1 for both Geode and Native Redis, so must be a Jedis bug
+ * TODO submit GitHub issue for this
Review comment:
This TODO comment was actually some leftover cruft and just needed to 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] jdeppe-pivotal merged pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #7340:
URL: https://github.com/apache/geode/pull/7340
--
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] ezoerner commented on a change in pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
ezoerner commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r808180787
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
##########
@@ -216,11 +216,22 @@ public void shouldReturnRange_givenLeftParenOnlyForMinOrMax() {
public void shouldReturnRange_boundedByLimit() {
createZSetRangeTestMap();
- assertThat(jedis.zrevrangeByScore(KEY, "10", "0", 0, 2))
+ /*
+ * Fails in Jedis 4.1.1 for both Geode and Native Redis, so must be a Jedis bug
+ * TODO submit GitHub issue for this
Review comment:
It's not clear to me that the localSocketAddress is actually necessary here. I think this may have been part of the thread name for debugging purposes or for trivial reasons. So I will delete the commented out code, but only file a bug if someone thinks it is necessary. The test works fine without it.
--
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] nonbinaryprogrammer commented on a change in pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r806346034
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
##########
@@ -216,11 +216,22 @@ public void shouldReturnRange_givenLeftParenOnlyForMinOrMax() {
public void shouldReturnRange_boundedByLimit() {
createZSetRangeTestMap();
- assertThat(jedis.zrevrangeByScore(KEY, "10", "0", 0, 2))
+ /*
+ * Fails in Jedis 4.1.1 for both Geode and Native Redis, so must be a Jedis bug
+ * TODO submit GitHub issue for this
Review comment:
You should probably go ahead and file this bug and remove this TODO
--
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 #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r806047523
##########
File path: geode-for-redis/src/commonTest/java/org/apache/geode/redis/mocks/MockSubscriber.java
##########
@@ -45,8 +43,9 @@
Collections.synchronizedList(new ArrayList<>());
private CountDownLatch messageReceivedLatch = new CountDownLatch(0);
private CountDownLatch pMessageReceivedLatch = new CountDownLatch(0);
- private String localSocketAddress;
- private Client client;
+ /*
+ * private String localSocketAddress;
+ */
Review comment:
Probably delete this.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -234,20 +235,20 @@ public void shouldUnionize_givenBoundaryScoresAndWeights() {
jedis.zunionstore(NEW_SET, new ZParams().weights(1), KEY1);
- Set<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, scores.size());
- assertThat(results).containsExactlyElementsOf(expectedResults);
+ List<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, scores.size());
+ assertThat(results).containsExactlyInAnyOrderElementsOf(expectedResults);
Review comment:
Please keep all of these assertions as `containsExactlyElementsOf` if possible (I ran a quick test and looks like it's OK). Since `ZRANGESCORE` is supposed to return an ordered list, we should check that.
##########
File path: geode-for-redis/src/commonTest/java/org/apache/geode/redis/mocks/MockSubscriber.java
##########
@@ -65,12 +64,24 @@ public MockSubscriber(CountDownLatch subscriptionLatch, CountDownLatch unsubscri
this.pUnsubscriptionLatch = pUnsubscriptionLatch;
}
- @Override
- public void proceed(Client client, String... channels) {
- localSocketAddress = client.getSocket().getLocalSocketAddress().toString();
- this.client = client;
- super.proceed(client, channels);
- }
+ /*
+ * @Override
+ * public void proceed(final Connection connection, final String... channels) {
+ * try {
+ * // Kludge due to socket becoming private in jedis 4.1.1
+ * // TODO is there a safe public way of getting local socket address
+ * // This doesn't work (or no longer works in Jedis 4.1.1), results in null socket
+ * final Field privateSocketField = Connection.class.getDeclaredField("socket");
+ * privateSocketField.setAccessible(true);
+ * final Socket socket = (Socket) privateSocketField.get(connection);
+ *
+ * localSocketAddress = socket.getLocalSocketAddress().toString();
+ * } catch (final NoSuchFieldException | IllegalAccessException ex) {
+ * throw new RuntimeException("Error in accessing private field 'socket' via reflection", ex);
+ * }
+ * super.proceed(connection, channels);
+ * }
+ */
Review comment:
Also 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 #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r807177685
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/pubsub/AbstractSubCommandsIntegrationTest.java
##########
@@ -293,8 +293,8 @@ public void numsub_shouldReturnSubscriberCount_whenCalledWithPatternAndSubscribe
@Test
public void numpat_shouldError_givenTooManyArguments() {
assertAtMostNArgsForSubCommand(introspector,
- Protocol.Command.PUBSUB,
- PUBSUB_NUM_PAT.getBytes(),
+ PUBSUB,
+ Protocol.Keyword.NUMPAT.toString().getBytes(),
Review comment:
This can be simplified to `NUMPAT.getRaw()`
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/server/ShutdownIntegrationTest.java
##########
@@ -53,10 +56,13 @@ public int getPort() {
@Test
public void shutdownIsDisabled_whenOnlySupportedCommandsAreAllowed() {
- // Unfortunately Jedis' shutdown() doesn't seem to throw a JedisDataException when the command
- // returns an error.
- jedis.shutdown();
+ final String EXPECTED_ERROR_MSG =
+ String.format(ERROR_UNKNOWN_COMMAND, "SHUTDOWN", "");
+ assertThatThrownBy(
+ () -> jedis.shutdown())
+ .isInstanceOf(JedisDataException.class)
+ .hasMessageContaining(EXPECTED_ERROR_MSG);
Review comment:
Could this assertion be changed to `hasMessage()` to make it more explicit about the exact message we expect to see here please?
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZInterStoreIntegrationTest.java
##########
@@ -253,7 +255,7 @@ public void shouldOverwriteDestinationKey_givenDestinationExists() {
jedis.zadd(KEY1, 1.0, "key1Member");
assertThat(jedis.zinterstore(NEW_SET, KEY1)).isEqualTo(1L);
- Set<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0L, 1L);
+ final List<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0L, 1L);
assertThat(results).containsExactlyInAnyOrderElementsOf(expectedResults);
Review comment:
With the change of the return type from Set to List, these assertions should be changed to `containsExactly()` since we expect that sorted sets preserve their ordering correctly in these tests.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZUnionStoreIntegrationTest.java
##########
@@ -234,20 +235,20 @@ public void shouldUnionize_givenBoundaryScoresAndWeights() {
jedis.zunionstore(NEW_SET, new ZParams().weights(1), KEY1);
- Set<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, scores.size());
- assertThat(results).containsExactlyElementsOf(expectedResults);
+ List<Tuple> results = jedis.zrangeWithScores(NEW_SET, 0, scores.size());
+ assertThat(results).containsExactlyInAnyOrderElementsOf(expectedResults);
Review comment:
These tests should also be using a List for their `expectedResults` rather than a set, since that's what Jedis returns now. This is another case where simple find/replace can make the conversion fairly painless.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZInterStoreIntegrationTest.java
##########
@@ -797,7 +799,7 @@ Tuple tupleMinOfScoresWithWeights(String memberName, Map<String, Double> scores1
@Test
public void test_assertThatActualScoresAreVeryCloseToExpectedScores() {
- Set<Tuple> actualResult = new HashSet<>(3);
+ List<Tuple> actualResult = new ArrayList<>(3);
Set<Tuple> expectedResult = new HashSet<>(2);
Review comment:
If the returned type from Jedis has changed from a Set to a List, then these tests should create their expected result as a List too. Luckily, in this class, this is a simple matter of doing a find/replace for `Set<Tuple>` -> `List<Tuple>` and `HashSet` and `LinkedHashSet` -> `ArrayList`.
##########
File path: geode-for-redis/src/commonTest/java/org/apache/geode/redis/mocks/MockSubscriber.java
##########
@@ -133,16 +144,6 @@ public void onPong(String pattern) {
receivedPings.add(pattern);
}
- // JedisPubSub ping with message is not currently possible, will submit a PR
- // (https://github.com/xetorthio/jedis/issues/2049)
- public void ping(String message) {
Review comment:
This method was previously being called in `AbstractSubscriptionsIntegrationTest` which has a comment referencing the issue of not being able to ping with a message. Since this issue has been resolved in Jedis 4.1.1, the comment in that test can be removed along with the extra call to `ping()`.
##########
File path: geode-for-redis/src/distributedTest/java/org/apache/geode/redis/internal/executor/string/StringsKillMultipleServersDUnitTest.java
##########
@@ -56,11 +56,16 @@ public static void classSetup() {
int redisServerPort1 = cluster.getRedisPort(1);
jedisCluster =
- new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort1), 10_000);
+ new JedisCluster(new HostAndPort(BIND_ADDRESS, redisServerPort1), 10_000, 20);
// This sequence ensures that servers 1, 2 and 3 are hosting all the buckets and server 4
// has no buckets.
cluster.startRedisVM(4, locatorPort);
+
+ cluster.enableDebugLogging(1);
+ cluster.enableDebugLogging(2);
+ cluster.enableDebugLogging(3);
+ cluster.enableDebugLogging(4);
Review comment:
This should probably be removed.
--
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] ezoerner commented on a change in pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
ezoerner commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r808180787
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
##########
@@ -216,11 +216,22 @@ public void shouldReturnRange_givenLeftParenOnlyForMinOrMax() {
public void shouldReturnRange_boundedByLimit() {
createZSetRangeTestMap();
- assertThat(jedis.zrevrangeByScore(KEY, "10", "0", 0, 2))
+ /*
+ * Fails in Jedis 4.1.1 for both Geode and Native Redis, so must be a Jedis bug
+ * TODO submit GitHub issue for this
Review comment:
It's not entirely clear to me that the localSocketAddress is actually necessary here. I think this may have been part of the thread name for debugging purposes or for trivial reasons. So I will delete the commented out code, but only file a bug if someone thinks it is necessary. The test works fine without it.
--
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] nonbinaryprogrammer commented on a change in pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #7340:
URL: https://github.com/apache/geode/pull/7340#discussion_r806346034
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/commands/executor/sortedset/AbstractZRevRangeByScoreIntegrationTest.java
##########
@@ -216,11 +216,22 @@ public void shouldReturnRange_givenLeftParenOnlyForMinOrMax() {
public void shouldReturnRange_boundedByLimit() {
createZSetRangeTestMap();
- assertThat(jedis.zrevrangeByScore(KEY, "10", "0", 0, 2))
+ /*
+ * Fails in Jedis 4.1.1 for both Geode and Native Redis, so must be a Jedis bug
+ * TODO submit GitHub issue for this
Review comment:
You should probably go ahead and file this bug and remove this TODO
--
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] onichols-pivotal edited a comment on pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
onichols-pivotal edited a comment on pull request #7340:
URL: https://github.com/apache/geode/pull/7340#issuecomment-1037048470
> > First commit (and also PR title) must begin with `GEODE-nnnnn: `. Please amend your initial commit and force-push.
>
> @onichols-pivotal I think this message might have been automatically generated in error, since the JIRA ticket number is GEODE-9998. Adding a zero to the front of the number is just likely to cause confusion, so I think this should probably be put back as it was.
Sorry if that was unclear. Not asking anything to be zero-padded! that would make no sense at all! Just asking that the ticket number be included at the beginning of the commit message FOLLOWED BY {COLON}{SPACE}. Looks like you are just missing the colon.
--
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 pull request #7340: GEODE-09998 upgrade Jedis client to 4.1.1
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on pull request #7340:
URL: https://github.com/apache/geode/pull/7340#issuecomment-1036989142
> First commit (and also PR title) must begin with `GEODE-nnnnn: `. Please amend your initial commit and force-push.
@onichols-pivotal I think this message might have been automatically generated in error, since the JIRA ticket number is GEODE-9998. Adding a zero to the front of the number is just likely to cause confusion, so I think this should probably be put back as it was.
--
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