[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/geode/pull/673


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130782011
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * 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.protocol.protobuf.operations;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.client.internal.locator.GetAllServersRequest;
+import 
org.apache.geode.cache.client.internal.locator.GetAllServersResponse;
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.tcpserver.TcpClient;
+import org.apache.geode.internal.admin.remote.DistributionLocatorId;
+import org.apache.geode.protocol.operations.OperationHandler;
+import org.apache.geode.protocol.protobuf.BasicTypes;
+import org.apache.geode.protocol.protobuf.Failure;
+import org.apache.geode.protocol.protobuf.Result;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import org.apache.geode.protocol.protobuf.Success;
+import org.apache.geode.serialization.SerializationService;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+public class GetAvailableServersOperationHandler implements
--- End diff --

@Experimental or a doc comment explaining why this won't be around forever 
would be awesome.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130781962
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * 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.protocol.protobuf.operations;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.client.internal.locator.GetAllServersRequest;
+import 
org.apache.geode.cache.client.internal.locator.GetAllServersResponse;
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.tcpserver.TcpClient;
+import org.apache.geode.internal.admin.remote.DistributionLocatorId;
+import org.apache.geode.protocol.operations.OperationHandler;
+import org.apache.geode.protocol.protobuf.BasicTypes;
+import org.apache.geode.protocol.protobuf.Failure;
+import org.apache.geode.protocol.protobuf.Result;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import org.apache.geode.protocol.protobuf.Success;
+import org.apache.geode.serialization.SerializationService;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+public class GetAvailableServersOperationHandler implements
+OperationHandler {
+
+  @Override
+  public Result process(
+  SerializationService serializationService, 
ServerAPI.GetAvailableServersRequest request,
+  Cache cache) {
+HashSet locators = 
getLocatorIdsFromDistributedProperties(cache);
+
+TcpClient tcpClient = getTcpClient();
+Optional result = 
locators.stream()
+.map((locatorId -> getGetAvailableServersFromLocator(tcpClient, 
locatorId)))
+.filter((obj) -> obj != null).findFirst();
+
+if (result.isPresent()) {
+  return result.get();
+} else {
+  return Failure
+  .of(BasicTypes.ErrorResponse.newBuilder().setMessage("Unable to 
find a locator").build());
+}
+  }
+
+  private HashSet 
getLocatorIdsFromDistributedProperties(Cache cache) {
--- End diff --

Will this work if locators are configured via cluster config? I'm guessing 
it won't work if the original locators are no longer in the cluster. Since this 
is a prototype and will be redone better, I think it's fine for now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130647800
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandlerJUnitTest.java
 ---
@@ -0,0 +1,131 @@
+/*
+ * 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.protocol.protobuf.operations;
+
+import 
org.apache.geode.cache.client.internal.locator.GetAllServersResponse;
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.tcpserver.TcpClient;
+import org.apache.geode.internal.cache.GemFireCacheImpl;
+import org.apache.geode.protocol.protobuf.BasicTypes;
+import org.apache.geode.protocol.protobuf.Failure;
+import org.apache.geode.protocol.protobuf.Result;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import 
org.apache.geode.protocol.protobuf.ServerAPI.GetAvailableServersResponse;
+import org.apache.geode.protocol.protobuf.Success;
+import 
org.apache.geode.protocol.protobuf.utilities.ProtobufRequestUtilities;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Properties;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+@Category(UnitTest.class)
+public class GetAvailableServersOperationHandlerJUnitTest extends 
OperationHandlerJUnitTest {
+
+  private TcpClient mockTCPClient;
+
+  @Before
+  public void setUp() throws Exception {
+super.setUp();
+
+operationHandler = mock(GetAvailableServersOperationHandler.class);
+cacheStub = mock(GemFireCacheImpl.class);
+when(operationHandler.process(any(), any(), 
any())).thenCallRealMethod();
+InternalDistributedSystem mockDistributedSystem = 
mock(InternalDistributedSystem.class);
--- End diff --

Can we have some blank lines as separators between segments that pertain to 
a single mock? Right now this is a fairly impenetrable wall of text.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130646564
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/GetAvailableServersDUnitTest.java
 ---
@@ -0,0 +1,108 @@
+/*
+ * 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.protocol;
+
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.protocol.exception.InvalidProtocolMessageException;
+import org.apache.geode.protocol.protobuf.ClientProtocol;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import 
org.apache.geode.protocol.protobuf.serializer.ProtobufProtocolSerializer;
+import 
org.apache.geode.protocol.protobuf.utilities.ProtobufRequestUtilities;
+import org.apache.geode.protocol.protobuf.utilities.ProtobufUtilities;
+import org.apache.geode.test.dunit.DistributedTestUtils;
+import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.cache.internal.JUnit4CacheTestCase;
+import 
org.apache.geode.test.dunit.rules.DistributedRestoreSystemProperties;
+import org.apache.geode.test.junit.categories.DistributedTest;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+import java.net.Socket;
+
+import static org.junit.Assert.assertEquals;
+
+@Category(DistributedTest.class)
+public class GetAvailableServersDUnitTest extends JUnit4CacheTestCase {
+
+  @Rule
+  public DistributedRestoreSystemProperties 
distributedRestoreSystemProperties =
+  new DistributedRestoreSystemProperties();
+
+  @Before
+  public void setup() {
+
+  }
+
+  @Test
+  public void testGetAllAvailableServersRequest()
+  throws IOException, InvalidProtocolMessageException {
+Host host = Host.getHost(0);
+VM vm0 = host.getVM(0);
+VM vm1 = host.getVM(1);
+VM vm2 = host.getVM(2);
+
+int locatorPort = DistributedTestUtils.getDUnitLocatorPort();
+
+// int cacheServer1Port = vm0.invoke("Start Cache1", () -> 
startCacheWithCacheServer());
--- End diff --

let's get rid off this


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130646340
  
--- Diff: geode-protobuf/src/main/proto/clientProtocol.proto ---
@@ -43,13 +43,12 @@ message Request {
 GetAllRequest getAllRequest = 5;
 RemoveRequest removeRequest = 6;
 RemoveAllRequest removeAllRequest = 7;
-ListKeysRequest listKeysRequest = 8;
 
 CreateRegionRequest createRegionRequest = 21;
 DestroyRegionRequest destroyRegionRequest = 22;
 
-PingRequest pingRequest = 41;
-GetServersRequest getServersRequest = 42;
+//PingRequest pingRequest = 41;
--- End diff --

what are you saving it for?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130646425
  
--- Diff: geode-protobuf/src/main/proto/clientProtocol.proto ---
@@ -64,15 +63,14 @@ message Response {
 GetAllResponse getAllResponse = 5;
 RemoveResponse removeResponse = 6;
 RemoveAllResponse removeAllResponse = 7;
-ListKeysResponse listKeysResponse = 8;
 
 ErrorResponse errorResponse = 13;
 
 CreateRegionResponse createRegionResponse = 20;
 DestroyRegionResponse destroyRegionResponse = 21;
 
-PingResponse pingResponse = 41;
-GetServersResponse getServersResponse = 42;
+//PingResponse pingResponse = 41;
--- End diff --

same as above


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130646177
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/utilities/ProtobufUtilities.java
 ---
@@ -140,7 +141,7 @@
 
   /**
* This creates a MessageHeader
-   *
+   * 
--- End diff --

what's up with adding all this white trailing white space? Is this from 
spotless?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130645759
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
 ---
@@ -0,0 +1,98 @@
+/*
+ * 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.protocol.protobuf.operations;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.client.internal.locator.GetAllServersRequest;
+import 
org.apache.geode.cache.client.internal.locator.GetAllServersResponse;
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.tcpserver.TcpClient;
+import org.apache.geode.internal.admin.remote.DistributionLocatorId;
+import org.apache.geode.protocol.operations.OperationHandler;
+import org.apache.geode.protocol.protobuf.BasicTypes;
+import org.apache.geode.protocol.protobuf.Failure;
+import org.apache.geode.protocol.protobuf.Result;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import org.apache.geode.protocol.protobuf.Success;
+import org.apache.geode.serialization.SerializationService;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Properties;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+public class GetAvailableServersOperationHandler implements
+OperationHandler {
+
+  @Override
+  public Result process(
+  SerializationService serializationService, 
ServerAPI.GetAvailableServersRequest request,
+  Cache cache) {
+
+InternalDistributedSystem distributedSystem =
+(InternalDistributedSystem) cache.getDistributedSystem();
+Properties properties = distributedSystem.getProperties();
+String locatorsString = 
properties.getProperty(ConfigurationProperties.LOCATORS);
+
+HashSet locators = new HashSet();
+StringTokenizer stringTokenizer = new StringTokenizer(locatorsString, 
",");
+while (stringTokenizer.hasMoreTokens()) {
+  String locator = stringTokenizer.nextToken();
+  if (StringUtils.isNotEmpty(locator)) {
+locators.add(new DistributionLocatorId(locator));
+  }
+}
+
+TcpClient tcpClient = getTcpClient();
+for (DistributionLocatorId locator : locators) {
+  try {
+return getGetAvailableServersFromLocator(tcpClient, 
locator.getHost());
+  } catch (IOException | ClassNotFoundException e) {
+// try the next locator
+  }
+}
+return Failure
+.of(BasicTypes.ErrorResponse.newBuilder().setMessage("Unable to 
find a locator").build());
+  }
+
+  private Result 
getGetAvailableServersFromLocator(
+  TcpClient tcpClient, InetSocketAddress address) throws IOException, 
ClassNotFoundException {
+GetAllServersResponse getAllServersResponse = (GetAllServersResponse) 
tcpClient
+.requestToServer(address, new GetAllServersRequest(), 1000, true);
+Collection servers =
+(Collection) 
getAllServersResponse.getServers().stream()
+.map(serverLocation -> 
getServerProtobufMessage((ServerLocation) serverLocation))
+.collect(Collectors.toList());
+ServerAPI.GetAvailableServersResponse.Builder builder =
+
ServerAPI.GetAvailableServersResponse.newBuilder().addAllServers(servers);
--- End diff --

could we incrementally build this up as part of the stream processing?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not 

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130644766
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
 ---
@@ -0,0 +1,98 @@
+/*
+ * 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.protocol.protobuf.operations;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.client.internal.locator.GetAllServersRequest;
+import 
org.apache.geode.cache.client.internal.locator.GetAllServersResponse;
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.tcpserver.TcpClient;
+import org.apache.geode.internal.admin.remote.DistributionLocatorId;
+import org.apache.geode.protocol.operations.OperationHandler;
+import org.apache.geode.protocol.protobuf.BasicTypes;
+import org.apache.geode.protocol.protobuf.Failure;
+import org.apache.geode.protocol.protobuf.Result;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import org.apache.geode.protocol.protobuf.Success;
+import org.apache.geode.serialization.SerializationService;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Properties;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+public class GetAvailableServersOperationHandler implements
+OperationHandler {
+
+  @Override
+  public Result process(
+  SerializationService serializationService, 
ServerAPI.GetAvailableServersRequest request,
+  Cache cache) {
+
+InternalDistributedSystem distributedSystem =
+(InternalDistributedSystem) cache.getDistributedSystem();
+Properties properties = distributedSystem.getProperties();
+String locatorsString = 
properties.getProperty(ConfigurationProperties.LOCATORS);
+
+HashSet locators = new HashSet();
+StringTokenizer stringTokenizer = new StringTokenizer(locatorsString, 
",");
+while (stringTokenizer.hasMoreTokens()) {
+  String locator = stringTokenizer.nextToken();
+  if (StringUtils.isNotEmpty(locator)) {
+locators.add(new DistributionLocatorId(locator));
+  }
+}
+
+TcpClient tcpClient = getTcpClient();
+for (DistributionLocatorId locator : locators) {
--- End diff --

this is not super pretty. What do you think about using Stream#findFirst 
for this?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/673#discussion_r130642543
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
 ---
@@ -0,0 +1,98 @@
+/*
+ * 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.protocol.protobuf.operations;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.client.internal.locator.GetAllServersRequest;
+import 
org.apache.geode.cache.client.internal.locator.GetAllServersResponse;
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.tcpserver.TcpClient;
+import org.apache.geode.internal.admin.remote.DistributionLocatorId;
+import org.apache.geode.protocol.operations.OperationHandler;
+import org.apache.geode.protocol.protobuf.BasicTypes;
+import org.apache.geode.protocol.protobuf.Failure;
+import org.apache.geode.protocol.protobuf.Result;
+import org.apache.geode.protocol.protobuf.ServerAPI;
+import org.apache.geode.protocol.protobuf.Success;
+import org.apache.geode.serialization.SerializationService;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Properties;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+public class GetAvailableServersOperationHandler implements
+OperationHandler {
+
+  @Override
+  public Result process(
+  SerializationService serializationService, 
ServerAPI.GetAvailableServersRequest request,
+  Cache cache) {
+
+InternalDistributedSystem distributedSystem =
+(InternalDistributedSystem) cache.getDistributedSystem();
+Properties properties = distributedSystem.getProperties();
+String locatorsString = 
properties.getProperty(ConfigurationProperties.LOCATORS);
--- End diff --

Could we pull the entire calculation of locators out either into a private 
method or a service object? I think that would make it much easier to 
understand what this method is doing. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #673: GEODE-3284: New flow: getAvailableServers

[WireBaron]

GitHub user WireBaron opened a pull request:

https://github.com/apache/geode/pull/673

GEODE-3284: New flow: getAvailableServers

@galen-pivotal @kohlmu-pivotal @bschuchardt @hiteshk25 @pivotal-amurmann 

Signed-off-by: Bruce Schuchardt 

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:
- [x] 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`)?

- [x] Is your initial contribution a single, squashed commit?

- [x] Does `gradlew build` run cleanly?

- [x] 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, you check travis-ci 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.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/WireBaron/geode feature/GEODE-3284

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/geode/pull/673.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #673


commit 6da72b504ed83d80fde60e8e661badab4d3d33bb
Author: Udo Kohlmeyer 
Date:   2017-07-31T16:23:53Z

GEODE-3284: New flow: getAvailableServers

Signed-off-by: Bruce Schuchardt 




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---