[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #7340: GEODE-9998: upgrade Jedis client to 4.1.1

[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