[GitHub] geode pull request #737: GEODE-3503: Removal of Codec classes left behind.

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/737#discussion_r134890133
  
--- Diff: geode-protobuf/src/main/proto/region_API.proto ---
@@ -58,6 +58,7 @@ message GetAllRequest {
 
 message GetAllResponse {
 repeated Entry entries = 1;
+repeated KeyedErrorResponse failedKeys = 2;
--- End diff --

This seems out of place in this change.


---
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 #716: GEODE-3406: Locator accepts Protobuf requests

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/716#discussion_r133599245
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ExecutionContext.java
 ---
@@ -0,0 +1,54 @@
+/*
+ * 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.cache.tier.sockets;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.internal.InternalLocator;
+
+public class ExecutionContext {
--- End diff --

Should this be marked experimental like the exception?


---
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 #716: GEODE-3406: Locator accepts Protobuf requests

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/716#discussion_r133598920
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAvailableServersOperationHandler.java
 ---
@@ -50,51 +37,19 @@
   @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));
-  }
-}
+  ExecutionContext executionContext) throws 
InvalidExecutionContextException {
 
-TcpClient tcpClient = getTcpClient();
-for (DistributionLocatorId locator : locators) {
-  try {
-return getGetAvailableServersFromLocator(tcpClient, 
locator.getHost());
-  } catch (IOException | ClassNotFoundException e) {
-// try the next locator
-  }
-}
-return Failure.of(ProtobufResponseUtilities.makeErrorResponse(
-ProtocolErrorCode.DATA_UNREACHABLE.codeValue, "Unable to find a 
locator"));
-  }
+InternalLocator locator = executionContext.getLocator();
+ArrayList servers2 = 
locator.getServerLocatorAdvisee().getLoadSnapshot().getServers(null);
--- End diff --

I see we didn't fix up this inane variable name I added, can you take care 
of that.


---
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 #716: GEODE-3406: Locator accepts Protobuf requests

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/716#discussion_r133596280
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
 ---
@@ -334,42 +342,46 @@ protected void run() {
* fix for bug 33711 - client requests are spun off to another thread 
for processing. Requests are
* synchronized in processGossip.
*/
-  private void processRequest(final Socket sock) {
+  private void processRequest(final Socket socket) {
 executor.execute(() -> {
   long startTime = DistributionStats.getStatTime();
   DataInputStream input = null;
   Object request, response;
   try {
 
-sock.setSoTimeout(READ_TIMEOUT);
-getSocketCreator().configureServerSSLSocket(sock);
+socket.setSoTimeout(READ_TIMEOUT);
+getSocketCreator().configureServerSSLSocket(socket);
 
 try {
-  input = new DataInputStream(sock.getInputStream());
+  input = new DataInputStream(socket.getInputStream());
 } catch (StreamCorruptedException e) {
   // Some garbage can be left on the socket stream
   // if a peer disappears at exactly the wrong moment.
   log.debug("Discarding illegal request from "
-  + (sock.getInetAddress().getHostAddress() + ":" + 
sock.getPort()), e);
+  + (socket.getInetAddress().getHostAddress() + ":" + 
socket.getPort()), e);
   return;
 }
-int gossipVersion = readGossipVersion(sock, input);
+int gossipVersion = readGossipVersion(socket, input);
 
 short versionOrdinal;
+if (gossipVersion == NON_GOSSIP_REQUEST_VERSION) {
+  if (input.readUnsignedByte() == 
AcceptorImpl.PROTOBUF_CLIENT_SERVER_PROTOCOL
+  && Boolean.getBoolean("geode.feature-protobuf-protocol")) {
+ClientProtocolMessageHandler messageHandler = 
ClientProtocolMessageHandlerLoader.load();
--- End diff --

Rather than introduce a new static object here, can we just have the 
TcpServer own a messageHandler?


---
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 #716: GEODE-3406: Locator accepts Protobuf requests

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/716#discussion_r133597359
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientProtoclMessageHandlerLoader.java
 ---
@@ -0,0 +1,64 @@
+/*
+ * 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.cache.tier.sockets;
+
+import java.io.IOException;
+import java.net.Socket;
+import java.util.Iterator;
+import java.util.ServiceLoader;
+
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.tier.Acceptor;
+import org.apache.geode.internal.cache.tier.CachedRegionHelper;
+import org.apache.geode.internal.security.SecurityService;
+
+/**
+ * Creates instances of ServerConnection based on the connection mode 
provided.
+ */
+public class ClientProtoclMessageHandlerLoader {
--- End diff --

Typo: s/Protocl/Protocol

Actually, this looks like a second copy of this code, so should just be 
deleted in favor of the correctly spelled version.


---
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 #716: GEODE-3406: Locator accepts Protobuf requests

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/716#discussion_r133598026
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ExecutionContext.java
 ---
@@ -0,0 +1,54 @@
+/*
+ * 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.cache.tier.sockets;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.internal.InternalLocator;
+
+public class ExecutionContext {
--- End diff --

Should this be marked @Experimental


---
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 #716: GEODE-3406: Locator accepts Protobuf requests

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/716#discussion_r133595959
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
 ---
@@ -120,6 +126,7 @@
   private InetAddress bind_address;
   private volatile boolean shuttingDown = false; // GemStoneAddition
   private final PoolStatHelper poolHelper;
+  private InternalLocator internalLocator;
--- End diff --

Can this be final?


---
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 #716: GEODE-3406: Locator accepts Protobuf requests

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/716#discussion_r133596604
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java ---
@@ -76,7 +76,9 @@
  */
 public interface InternalCache extends Cache, Extensible, CacheTime 
{
 
-  InternalDistributedMember getMyId();
+  default InternalDistributedMember getMyId() {
--- End diff --

I don't remember why this got done, is it still necessary?


---
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 #716: GEODE-3406: Locator accepts Protobuf requests

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/716#discussion_r133597935
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ExecutionContext.java
 ---
@@ -0,0 +1,54 @@
+/*
+ * 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.cache.tier.sockets;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.distributed.internal.InternalLocator;
+
+public class ExecutionContext {
+  private Cache cache;
+  private InternalLocator locator;
+
+  public ExecutionContext(Cache cache) {
+this.cache = cache;
+  }
+
+  public ExecutionContext(InternalLocator locator) {
+this.locator = locator;
+  }
+
+  // This throws if the cache isn't present because we know that non of 
the callers can take any
+  // reasonable action if the cache is not present
+  public Cache getCache() throws InvalidExecutionContextException {
+if (cache != null) {
+  return cache;
+} else {
+  throw new InvalidExecutionContextException(
+  "Execution context's cache was accessed but isn't present. Did 
this happen on a locator? Operations on the locator should not try to operate 
on a cache");
+}
+  }
+
+  // This throws if the locator isn't present because we know that non of 
the callers can take any
+  // reasonable action if the locator is not present
+  public InternalLocator getLocator() throws 
InvalidExecutionContextException {
+if (locator != null) {
+  return locator;
+} else {
+  throw new InvalidExecutionContextException(
+  "Execution context's locator was accessed but isn't present. Did 
this happen on a server? Operations on the locator should not try to operate on 
a cache");
--- End diff --

Update comment "Operations on the server should not to try to operate on a 
locator.


---
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 #714: GEODE-3412: adding files missing from last commit

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3412: adding files missing from last commit

This is adding some files which should have been in the last commit for 
this feature.

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?

- [x] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [x] 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, 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/GEODE3412

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

https://github.com/apache/geode/pull/714.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 #714


commit 1022f323a0061f43155bd5ea74879981b8bc8ffc
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-08-15T18:21:51Z

GEODE-3412: adding files missing from last commit




---
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 #713: Feature/geode 3412

[WireBaron]

Github user WireBaron closed the pull request at:

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


---
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 #713: Feature/geode 3412

[WireBaron]

GitHub user WireBaron opened a pull request:

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

Feature/geode 3412

This is adding some files which should have been in the last commit for 
this feature.

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, 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-3412

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

https://github.com/apache/geode/pull/713.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 #713


commit daa140875c4f65838dde4ae0f627a9551bea7516
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-08-10T18:16:25Z

GEODE-3412: Add simple authentication flow to protobuf protocol.

This change adds a simple username/password validation to the protobuf 
protocol.
It also adds a new configuration parameter to specify the type of 
authentication required.

Signed-off-by: Galen O'Sullivan <gosulli...@pivotal.io>

commit 6753916687b9d916e18c3be5959f4c6aff490800
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-08-14T17:29:21Z

GEODE-3412: implementing review feedback

commit b11c02a59cb2d71d4ac7d0b821ca668c825e2f6c
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-08-15T17:09:39Z

GEODE-3412: moving authenticator interface to geode.security package

commit 704b6953e01a152e6859d4b254f4d8d39bd40011
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-08-15T18:21:51Z

GEODE-3412: adding files missing from last commit




---
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 #702: GEODE-3416: Reduce synchronization blockages in Soc...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/702#discussion_r133230687
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/net/SocketCloser.java ---
@@ -144,35 +156,22 @@ private boolean isClosed() {
* called then the asyncClose will be done synchronously.
*/
   public void close() {
-synchronized (asyncCloseExecutors) {
+synchronized (closed) {
   if (!this.closed) {
 this.closed = true;
-for (ThreadPoolExecutor pool : asyncCloseExecutors.values()) {
-  pool.shutdown();
-}
-asyncCloseExecutors.clear();
+  } else {
+return;
   }
 }
+for (ExecutorService executorService : asyncCloseExecutors.values()) {
+  executorService.shutdown();
+  asyncCloseExecutors.clear();
--- End diff --

Shouldn't this be outside the for loop?


---
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 #702: GEODE-3416: Reduce synchronization blockages in Soc...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/702#discussion_r133230527
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/net/SocketCloser.java ---
@@ -96,46 +99,55 @@ public int getMaxThreads() {
 return this.asyncClosePoolMaxThreads;
   }
 
-  private ThreadPoolExecutor getAsyncThreadExecutor(String address) {
-synchronized (asyncCloseExecutors) {
-  ThreadPoolExecutor pool = asyncCloseExecutors.get(address);
-  if (pool == null) {
-final ThreadGroup tg = 
LoggingThreadGroup.createThreadGroup("Socket asyncClose", logger);
-ThreadFactory tf = new ThreadFactory() {
-  public Thread newThread(final Runnable command) {
-Thread thread = new Thread(tg, command);
-thread.setDaemon(true);
-return thread;
-  }
-};
-BlockingQueue workQueue = new 
LinkedBlockingQueue();
-pool = new ThreadPoolExecutor(this.asyncClosePoolMaxThreads, 
this.asyncClosePoolMaxThreads,
-this.asyncClosePoolKeepAliveSeconds, TimeUnit.SECONDS, 
workQueue, tf);
-pool.allowCoreThreadTimeOut(true);
-asyncCloseExecutors.put(address, pool);
+  private ExecutorService getAsyncThreadExecutor(String address) {
+ExecutorService executorService = asyncCloseExecutors.get(address);
+if (executorService == null) {
+  // To be used for pre-1.8 jdk releases.
+  // createThreadPool();
+
+  executorService = 
Executors.newWorkStealingPool(asyncClosePoolMaxThreads);
+
+  ExecutorService previousThreadPoolExecutor =
+  asyncCloseExecutors.putIfAbsent(address, executorService);
+
+  if (previousThreadPoolExecutor != null) {
+executorService.shutdownNow();
+return previousThreadPoolExecutor;
   }
-  return pool;
 }
+return executorService;
+  }
+
+  /**
+   * @deprecated this method is to be used for pre 1.8 jdk.
+   */
+  @Deprecated
+  private void createThreadPool() {
+ExecutorService executorService;
+final ThreadGroup threadGroup =
+LoggingThreadGroup.createThreadGroup("Socket asyncClose", logger);
+ThreadFactory threadFactory = new ThreadFactory() {
+  public Thread newThread(final Runnable command) {
+Thread thread = new Thread(threadGroup, command);
+thread.setDaemon(true);
+return thread;
+  }
+};
+
+executorService = new ThreadPoolExecutor(asyncClosePoolMaxThreads, 
asyncClosePoolMaxThreads,
+asyncCloseWaitTime, asyncCloseWaitUnits, new 
LinkedBlockingQueue<>(), threadFactory);
   }
 
   /**
* Call this method if you know all the resources in the closer for the 
given address are no
* longer needed. Currently a thread pool is kept for each address and 
if you know that an address
* no longer needs its pool then you should call this method.
*/
-  public void releaseResourcesForAddress(String address) {
-synchronized (asyncCloseExecutors) {
-  ThreadPoolExecutor pool = asyncCloseExecutors.get(address);
-  if (pool != null) {
-pool.shutdown();
-asyncCloseExecutors.remove(address);
-  }
-}
-  }
 
-  private boolean isClosed() {
-synchronized (asyncCloseExecutors) {
-  return this.closed;
+  public void releaseResourcesForAddress(String address) {
+ExecutorService executorService = asyncCloseExecutors.remove(address);
+if (executorService != null) {
+  executorService.shutdown();
--- End diff --

This could still race with a thread running the close() method below and 
both could end up calling shutdown on the same service.


---
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 #700: GEODE-3386 - Make KeyedErrorResponse & ErrorRespons...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/700#discussion_r132591028
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
 ---
@@ -41,9 +42,8 @@
 String regionName = request.getRegionName();
 Region region = cache.getRegion(regionName);
 if (region == null) {
-  return Failure.of(BasicTypes.ErrorResponse.newBuilder()
-  
.setErrorCode(ProtocolErrorCode.REGION_NOT_FOUND.codeValue).setMessage("Region 
not found")
-  .build());
+  return Failure.of(ProtobufResponseUtilities
+  .makeErrorResponse(ProtocolErrorCode.REGION_NOT_FOUND.codeValue, 
"Region not found"));
--- End diff --

Thank you for making this change, this has been bothering me since I added 
the error code. Sorry I didn't do this sooner.


---
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 #700: GEODE-3386 - Make KeyedErrorResponse & ErrorRespons...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/700#discussion_r132591701
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
 ---
@@ -28,6 +28,8 @@
 import org.apache.geode.serialization.SerializationService;
 import 
org.apache.geode.serialization.exception.UnsupportedEncodingTypeException;
 import 
org.apache.geode.serialization.registry.exception.CodecNotRegisteredForTypeException;
+
+import com.fasterxml.jackson.databind.introspect.TypeResolutionContext;
--- End diff --

I don't see how this new import is being used?


---
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 #700: GEODE-3386 - Make KeyedErrorResponse & ErrorRespons...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/700#discussion_r132591361
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/GetAllRequestOperationHandler.java
 ---
@@ -59,13 +59,14 @@
   }
   return 
Success.of(RegionAPI.GetAllResponse.newBuilder().addAllEntries(entries).build());
 } catch (UnsupportedEncodingTypeException ex) {
-  return Failure.of(BasicTypes.ErrorResponse.newBuilder()
-  .setErrorCode(ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue)
-  .setMessage("Encoding not supported.").build());
+  int errorCode = ProtocolErrorCode.VALUE_ENCODING_ERROR.codeValue;
+  String message = "Encoding not supported.";
+  return 
Failure.of(ProtobufResponseUtilities.makeErrorResponse(errorCode, message));
--- End diff --

Having a local variable for the error code and message or inlining them are 
both fine approaches, but it's a bit jarring to see both approaches used in two 
adjacent code blocks.  Please use a consistent approach.


---
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 #700: GEODE-3386 - Make KeyedErrorResponse & ErrorRespons...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/700#discussion_r132591888
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandler.java
 ---
@@ -79,9 +81,10 @@
   private BasicTypes.KeyedErrorResponse 
buildAndLogKeyedError(BasicTypes.Entry entry,
   ProtocolErrorCode errorCode, String message, Exception ex) {
 logger.error(message, ex);
-BasicTypes.ErrorResponse errorResponse = 
BasicTypes.ErrorResponse.newBuilder()
-.setErrorCode(errorCode.codeValue).setMessage(message).build();
-return 
BasicTypes.KeyedErrorResponse.newBuilder().setKey(entry.getKey()).setError(errorResponse)
+
+return 
BasicTypes.KeyedErrorResponse.newBuilder().setKey(entry.getKey())
+.setError(
+
BasicTypes.Error.newBuilder().setErrorCode(errorCode.codeValue).setMessage(message))
--- End diff --

Ugh...is this how spotless formatted 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 #707: GEODE-3412: Add simple authentication flow to proto...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3412: Add simple authentication flow to protobuf protocol.

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

This change adds a simple username/password validation to the protobuf 
protocol.
It also adds a new configuration parameter to specify the type of 
authentication required.

Signed-off-by: Galen O'Sullivan <gosulli...@pivotal.io>

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-3412

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

https://github.com/apache/geode/pull/707.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 #707


commit daa140875c4f65838dde4ae0f627a9551bea7516
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-08-10T18:16:25Z

GEODE-3412: Add simple authentication flow to protobuf protocol.

This change adds a simple username/password validation to the protobuf 
protocol.
It also adds a new configuration parameter to specify the type of 
authentication required.

Signed-off-by: Galen O'Sullivan <gosulli...@pivotal.io>




---
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 #683: GEODE-3314 - additional refactoring for developer Q...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/683#discussion_r131287954
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java
 ---
@@ -302,14 +302,14 @@ boolean checkForExpiration() {
* @param remoteThread identity of the leasing thread
* @return true if lease for this lock token is successfully granted
*/
-  synchronized boolean grantLock(long newLeaseExpireTime, int newLeaseId, 
int newRecursion,
+  synchronized void grantLock(long newLeaseExpireTime, int newLeaseId, int 
newRecursion,
   RemoteThread remoteThread) {
 
 Assert.assertTrue(remoteThread != null);
 Assert.assertTrue(newLeaseId > -1, "Invalid attempt to grant lock with 
leaseId " + newLeaseId);
 
 checkDestroyed();
-checkForExpiration();
+checkForExpiration(); // TODO: this should throw.
--- End diff --

Any chance this TODO can be implemented 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 #683: GEODE-3314 - additional refactoring for developer Q...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/683#discussion_r131287871
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockToken.java
 ---
@@ -87,7 +87,8 @@
   private Thread thread;
 
   /**
-   * Number of threads currently using this lock token.
+   * Number of usages of this lock token. usageCount = recursion + (# of 
threads waiting for this
+   * lock). It's weird, I know.
--- End diff --

Did you want the editorial comment pushed?


---
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 #683: GEODE-3314 - additional refactoring for developer Q...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/683#discussion_r131287366
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/locks/DLockRequestProcessor.java
 ---
@@ -196,6 +196,13 @@ long getLeaseExpireTime() {
 return this.response.leaseExpireTime;
   }
 
+  /**
+   *
+   * @param interruptible
+   * @param lockId
+   * @return
+   * @throws InterruptedException only possible if interruptible is true.
+   */
   protected boolean requestLock(boolean interruptible, int lockId) throws 
InterruptedException {
 final boolean isDebugEnabled_DLS = 
logger.isTraceEnabled(LogMarker.DLS);
--- End diff --

Did you want to remove this, as you seem to be removing a lot of switching 
based on it.  Is this code going to be log spammy without 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 issue #676: GEODE-3321: Adding ErrorCode values to protobuf protocol

[WireBaron]

Github user WireBaron commented on the issue:

https://github.com/apache/geode/pull/676
  
Rebased to fix conflicts and added an error code to the new 
GetAvailableServers handler.


---
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 #676: GEODE-3321: Adding ErrorCode values to protobuf pro...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3321: Adding ErrorCode values to protobuf protocol

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

Signed-off-by: Bruce Schuchardt <bschucha...@pivotal.io>

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?

- [x] 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-3321

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

https://github.com/apache/geode/pull/676.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 #676


commit 5a73db96aadd8542e90dccf9f164aa309e1ca3d2
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-08-01T23:56:24Z

GEODE-3321: Adding ErrorCode values to protobuf protocol

Signed-off-by: Bruce Schuchardt <bschucha...@pivotal.io>
Signed-off-by: Brian Rowe <br...@pivotal.io>




---
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 <bschucha...@pivotal.io>

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 <ukohlme...@pivotal.io>
Date:   2017-07-31T16:23:53Z

GEODE-3284: New flow: getAvailableServers

Signed-off-by: Bruce Schuchardt <bschucha...@pivotal.io>




---
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 issue #657: GEODE-3286: Failing to cleanup connections from Connection...

[WireBaron]

Github user WireBaron commented on the issue:

https://github.com/apache/geode/pull/657
  
Pushed a new revision tightening the check before adding a connection to 
the connectionTable's receivers.


---
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 #657: GEODE-3286: Failing to cleanup connections from Con...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/657#discussion_r129961462
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java ---
@@ -1322,6 +1328,14 @@ private void createBatchSendBuffer() {
 this.batchFlusher.start();
   }
 
+  public void onIdleCancel() {
--- End diff --

This was changed in the second commit.


---
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 #657: GEODE-3286: Failing to cleanup connections from Con...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/657#discussion_r129961225
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java ---
@@ -279,26 +280,29 @@ protected void acceptConnection(Socket sock) throws 
IOException, ConnectionExcep
   // in our caller.
   // no need to log error here since caller will log warning
 
-  if (conn != null && !finishedConnecting) {
+  if (connection != null && !finishedConnecting) {
 // we must be throwing from checkCancelInProgress so close the 
connection
-
closeCon(LocalizedStrings.ConnectionTable_CANCEL_AFTER_ACCEPT.toLocalizedString(),
 conn);
-conn = null;
+
closeCon(LocalizedStrings.ConnectionTable_CANCEL_AFTER_ACCEPT.toLocalizedString(),
+connection);
+connection = null;
   }
 }
 
-if (conn != null) {
+if (connection != null) {
   synchronized (this.receivers) {
-this.owner.stats.incReceivers();
+this.owner.getStats().incReceivers();
 if (this.closed) {
   
closeCon(LocalizedStrings.ConnectionTable_CONNECTION_TABLE_NO_LONGER_IN_USE
-  .toLocalizedString(), conn);
+  .toLocalizedString(), connection);
   return;
 }
-this.receivers.add(conn);
+if (!connection.isSocketClosed()) {
--- End diff --

Looks like there may be an issue here since the socket close is done 
asynchronously from the connection close().  I've changed the check to also 
check connection.stopped, which is set to false inside the close call, before 
the connection cleans up it's state.


---
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 #661: GEODE-3319 - refactor to use protobuf encoding for ...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3319 - refactor to use protobuf encoding for primitive types

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

This changes protobuf EncodedValue messages to store data formats that 
protobuf can natively recognize as those types and only use byte encoding for 
more complicated object types.

Signed-off-by: Brian Rowe <br...@pivotal.io>
Signed-off-by: Hitesh Khamesra <hitesh...@yahoo.com>
Signed-off-by: Udo Kohlmeyer <u...@apache.org>

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?

- [x] 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-3319

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

https://github.com/apache/geode/pull/661.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 #661


commit 48aadab39a5f3cf1714a40c743dc9f2f0590bc3a
Author: Hitesh Khamesra <hitesh...@yahoo.com>
Date:   2017-07-27T00:49:27Z

GEODE-3319 - refactor to use protobuf encoding for primitive types

Signed-off-by: Hitesh Khamesra <hitesh...@yahoo.com>
Signed-off-by: Udo Kohlmeyer <u...@apache.org>
Signed-off-by: Brian Rowe <br...@pivotal.io>




---
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 #657: GEODE-3286: Failing to cleanup connections from Con...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/657#discussion_r129683975
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java ---
@@ -1322,6 +1328,14 @@ private void createBatchSendBuffer() {
 this.batchFlusher.start();
   }
 
+  public void onIdleCancel() {
--- End diff --

We'll change this to cleanUpOnIdleTaskCancel.  The connection will always 
be closed when we reach this code.  We debated adding an assert here, but 
decided against it.


---
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 #657: GEODE-3286: Failing to cleanup connections from Con...

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/657#discussion_r129683563
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java ---
@@ -568,6 +568,12 @@ protected Connection(ConnectionTable t, Socket socket) 
throws IOException, Conne
 }
   }
 
+  protected void initRecevier() {
--- End diff --

fixed


---
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 #657: GEODE-3286: Failing to cleanup connections from Con...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3286: Failing to cleanup connections from ConnectionTable recei…

…ver table

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

- prevent adding a closed connection to the connection table's receivers
- add a new unit test for connection table
- adding connection factory object for creating receiving connections
- have the idle connection timeout ensure connections are removed from 
connection
  table receivers
- modify tcpConduit stat accesses to allow easier mocking

Signed-off-by: Hitesh Khamesra <hitesh...@yahoo.com>

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?

- [x] 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-3286

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

https://github.com/apache/geode/pull/657.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 #657


commit 8aed26846de6e9ff1c123acae98a7b5ce6d82a83
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-07-25T22:43:35Z

GEODE-3286: Failing to cleanup connections from ConnectionTable receiver 
table

- prevent adding a closed connection to the connection table's receivers
- add a new unit test for connection table
- adding connection factory object for creating receiving connections
- have the idle connection timeout ensure connections are removed from 
connection
  table receivers
- modify tcpConduit stat accesses to allow easier mocking

Signed-off-by: Hitesh Khamesra <hitesh...@yahoo.com>




---
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 #649: GEODE-2997: Change new protocol GetAllResponse.

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/649#discussion_r128652774
  
--- Diff: geode-protobuf/src/main/proto/basicTypes.proto ---
@@ -62,4 +62,14 @@ message Region {
 
 message Server {
 string url = 1;
-}
\ No newline at end of file
+}
+
+message Error {
--- End diff --

These are the same fields as ErrorResponse and KeyedErrorResponse in the 
outstanding PutAll pull request, can you comment in that PR to change the names 
if you prefer these?


---
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 #646: GEODE-3213: Refactor ProtoBuf handler flow

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128591891
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/operations/Result.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.protocol.operations;
+
+import org.apache.geode.protocol.protobuf.ClientProtocol;
+
+import java.util.function.Function;
+
+public interface Result {
--- End diff --

This is tightly bound to the ClientProtocol.ErrorResponse, can we give it a 
less generic name?  Maybe ProtocolHandlerResponse?


---
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 #646: GEODE-3213: Refactor ProtoBuf handler flow

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128597628
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/operations/Failure.java 
---
@@ -0,0 +1,47 @@
+/*
+ * 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.operations;
--- End diff --

In terms of package layout, I had thought the intent was to have generic 
code that could be used for any protocol in this package, while protobuf 
specific code would be under org.apache.geode.protocol.protobuf.*  All of the 
Result and OperationContext code is now protobuf specific.  Should it be moved?


---
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 #646: GEODE-3213: Refactor ProtoBuf handler flow

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/646#discussion_r128600182
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java
 ---
@@ -15,32 +15,35 @@
 package org.apache.geode.protocol.protobuf;
 
 import org.apache.geode.cache.Cache;
-import org.apache.geode.protocol.exception.InvalidProtocolMessageException;
-import org.apache.geode.protocol.operations.OperationHandler;
-import 
org.apache.geode.protocol.operations.registry.OperationsHandlerRegistry;
-import 
org.apache.geode.protocol.operations.registry.exception.OperationHandlerNotRegisteredException;
+import org.apache.geode.protocol.operations.OperationContext;
+import org.apache.geode.protocol.operations.Result;
+import 
org.apache.geode.protocol.operations.registry.OperationContextRegistry;
 import org.apache.geode.serialization.SerializationService;
 
 /**
  * This handles protobuf requests by determining the operation type of the 
request and dispatching
  * it to the appropriate handler.
  */
 public class ProtobufOpsProcessor {
-  private final OperationsHandlerRegistry opsHandlerRegistry;
+
+  private final OperationContextRegistry operationContextRegistry;
   private final SerializationService serializationService;
 
-  public ProtobufOpsProcessor(OperationsHandlerRegistry opsHandlerRegistry,
-  SerializationService serializationService) {
-this.opsHandlerRegistry = opsHandlerRegistry;
+  public ProtobufOpsProcessor(SerializationService serializationService,
+  OperationContextRegistry operationsContextRegistry) {
 this.serializationService = serializationService;
+this.operationContextRegistry = operationsContextRegistry;
   }
 
-  public ClientProtocol.Response process(ClientProtocol.Request request, 
Cache cache)
-  throws OperationHandlerNotRegisteredException, 
InvalidProtocolMessageException {
+  public ClientProtocol.Response process(ClientProtocol.Request request, 
Cache cache) {
 ClientProtocol.Request.RequestAPICase requestType = 
request.getRequestAPICase();
-OperationHandler opsHandler =
-
opsHandlerRegistry.getOperationHandlerForOperationId(requestType.getNumber());
+OperationContext operationContext = 
operationContextRegistry.getOperationContext(requestType);
--- End diff --

How are we dealing with missing registry entries now that we're no longer 
throwing OperationNotRegisteredException?


---
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 #643: GEODE-3192,GEODE-3229: Change API and implementatio...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3192,GEODE-3229: Change API and implementation of protobuf PutAll.

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

* We will now dispatch incoming protobuf PutAlls as a series of put 
operations
* The PutAllResponse will contain a set of failed keys and the error they 
failed with

Signed-off-by: Galen O'Sullivan <gosulli...@pivotal.io>

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?

- [x] 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-3229

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

https://github.com/apache/geode/pull/643.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 #643


commit a056814b30f76572dca68abea57895d4be670da2
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-07-18T21:52:50Z

GEODE-3192,GEODE-3229: Change API and implementation of protobuf PutAll.

* We will now dispatch incoming protobuf PutAlls as a series of put 
operations
* The PutAllResponse will contain a set of failed keys and the error they 
failed with

Signed-off-by: Galen O'Sullivan <gosulli...@pivotal.io>
Signed-off-by: Brian Rowe <br...@pivotal.io>




---
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 #632: GEODE-3203: fixing protobuf build.gradle to respect...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3203: fixing protobuf build.gradle to respect buildRoot

Signed-off-by: Bruce Schuchardt <bschucha...@pivotal.io>

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?

- [x] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [x] 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, 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-3203

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

https://github.com/apache/geode/pull/632.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 #632


commit 9322e6dfdf3d62e14d2c2f97a83ced558333883e
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-07-13T21:41:43Z

GEODE-3203: fixing protobuf build.gradle to respect buildRoot

Signed-off-by: Bruce Schuchardt <bschucha...@pivotal.io>




---
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 #630: GEODE-3141: GetRegion Operation implemented

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127299289
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java
 ---
@@ -14,6 +14,14 @@
  */
 package org.apache.geode.protocol.protobuf.operations;
 
+import static org.mockito.Mockito.any;
--- End diff --

spotlessApply doesn't seem to care about the import order, however IntelliJ 
will reorder according the style guide every time you run "optimize imports".  
Patrick was flagging every pull request with the outdated order for a while.


---
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 #630: GEODE-3141: GetRegion Operation implemented

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127292855
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/protobuf/operations/PutAllRequestOperationHandlerJUnitTest.java
 ---
@@ -14,6 +14,14 @@
  */
 package org.apache.geode.protocol.protobuf.operations;
 
+import static org.mockito.Mockito.any;
--- End diff --

Have you updated your IntelliJ style guide file with the June 13 version?  
They've modified the order of imports somewhat.


---
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 #630: GEODE-3141: GetRegion Operation implemented

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127290974
  
--- Diff: 
geode-protobuf/src/test/java/org/apache/geode/protocol/RoundTripCacheConnectionJUnitTest.java
 ---
@@ -278,6 +278,31 @@ public void 
useSSL_testNewProtocolHeaderLeadsToNewProtocolServerConnection() thr
 testNewProtocolHeaderLeadsToNewProtocolServerConnection();
   }
 
+  @Test
+  public void testNewProtocolGetRegionCallSucceeds() throws Exception {
+System.setProperty("geode.feature-protobuf-protocol", "true");
+
+Socket socket = new Socket("localhost", cacheServerPort);
+Awaitility.await().atMost(5, 
TimeUnit.SECONDS).until(socket::isConnected);
+OutputStream outputStream = socket.getOutputStream();
+outputStream.write(110);
+
+
+ProtobufProtocolSerializer protobufProtocolSerializer = new 
ProtobufProtocolSerializer();
+ClientProtocol.Message getRegionMessage =
+MessageUtil.makeGetRegionRequestMessage(TEST_REGION, 
ClientProtocol.MessageHeader.newBuilder().build());
+protobufProtocolSerializer.serialize(getRegionMessage, outputStream);
+
+ClientProtocol.Message message =
+protobufProtocolSerializer.deserialize(socket.getInputStream());
+assertEquals(ClientProtocol.Message.MessageTypeCase.RESPONSE, 
message.getMessageTypeCase());
+ClientProtocol.Response response = message.getResponse();
+assertEquals(ClientProtocol.Response.ResponseAPICase.GETREGIONRESPONSE,
+response.getResponseAPICase());
+response.getGetRegionResponse();
+//TODO add some assertions for Region data
--- End diff --

Can we resolve this TODO before pushing 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 #630: GEODE-3141: GetRegion Operation implemented

[WireBaron]

Github user WireBaron commented on a diff in the pull request:

https://github.com/apache/geode/pull/630#discussion_r127285451
  
--- Diff: 
geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/operations/RemoveRequestOperationHandler.java
 ---
@@ -33,7 +33,7 @@
 
   @Override
   public ClientProtocol.Response process(SerializationService 
serializationService,
-  ClientProtocol.Request request, Cache cache) {
+ ClientProtocol.Request request, 
Cache cache) {
--- End diff --

Does this pass spotless?  I thought all of these wonky indentations were 
actually introduced by spotlessApply?


---
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 issue #631: GEODE-3051: Remove unreachable exception handling in Accep...

[WireBaron]

Github user WireBaron commented on the issue:

https://github.com/apache/geode/pull/631
  
Isn't it the same code handling the SSL handshake for both protocols right
now? If it's a different code path, then we should probably add a new test
for it.

On Thu, Jul 13, 2017 at 8:37 AM, galen-pivotal <notificati...@github.com>
wrote:

> *@galen-pivotal* approved this pull request.
>
> It looks like CacheServerSSLConnectionDUnitTest.testNonSSLClient tests
> that a client using the old client protocol gets an SSL exception. Do you
> think it's worth making a test for the new protocol as well?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/geode/pull/631#pullrequestreview-49818299>, or 
mute
> the thread
> 
<https://github.com/notifications/unsubscribe-auth/AQGwNsVsxyM0gKTQY25WiKBFDqHxrrv2ks5sNjmngaJpZM4OWXjN>
> .
>



---
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 #631: GEODE-3051: Remove unreachable exception handling i...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3051: Remove unreachable exception handling in AcceptorImpl.accept

This removes handling of SSL exceptions from the AccepterImpl.accept call, 
as the SSL handling code is now all done in another thread.
The exception handling being done in the other thread appears to be 
correct, as validated by CacheServerSSLConnectionDUnitTest.testNonSSLClient

Signed-off-by: Brian Rowe <br...@pivotal.io>

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?

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

- [ ] 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-3051

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

https://github.com/apache/geode/pull/631.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 #631


commit fa922141344ba1bddb6fc7203e23a886af8d93fa
Author: Alexander Murmann <amurm...@pivotal.io>
Date:   2017-07-13T00:16:10Z

GEODE-3051: Remove unreachable exception handling in AcceptorImpl.accept

This removes handling of SSL exceptions from the AccepterImpl.accept call, 
as the SSL handling code is now all done in another thread.
The exception handling being done in the other thread appears to be 
correct, as validated by CacheServerSSLConnectionDUnitTest.testNonSSLClient

Signed-off-by: Brian Rowe <br...@pivotal.io>




---
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 #629: GEODE-2997: New flow getAll/putAll

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-2997: New flow getAll/putAll

Changed get response to indicate if LookupFailure was a missing key or key 
with null value, added test
Added GetAllRequestOperationHandler and unit test
Added PutAllRequestOperationHandler and unit test
Added an integration test covering the putAll and getAll operations
Removed serverInternal and retriable fields from ErrorResponse, replaced 
with errorCode field

Signed-off-by: Brian Rowe <br...@pivotal.io>

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?

- [x] 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-2997

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

https://github.com/apache/geode/pull/629.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 #629


commit 510d579788abdd04e49b6ee7748fe0953e972e99
Author: Alexander Murmann <amurm...@pivotal.io>
Date:   2017-07-06T00:36:20Z

GEODE-2997: New flow getAll/putAll

Changed get response to indicate if LookupFailure was a missing key or key 
with null value, added test
Added GetAllRequestOperationHandler and unit test
Added PutAllRequestOperationHandler and unit test
Added an integration test covering the putAll and getAll operations
Removed serverInternal and retriable fields from ErrorResponse, replaced 
with errorCode field

Signed-off-by: Brian Rowe <br...@pivotal.io>




---
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 #621: GEODE-3129 - Added error messages to protobuf proto...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3129 - Added error messages to protobuf protocol

added a new ErrorResponse type to ClientProtocol
removed success field from several RegionAPI response objects and 
refactored operation handlers to instead return ErrorResponses
made all op handlers take ClientProtocol.Requests and return 
ClientProtocol.Responses instead of operation specific types
moved the protobuf specific response building code from operation handlers 
to ProtobufResponseUtilities
moved the request building functions from MessageUtil (under Test) to 
ProtobufRequestUtilities
moved all utility classes to ...protocol.protobuf.utilities and added 
javadoc comments throughout
changed GetRegions to GetRegionNames, returns strings instead of Regions
replaced logging through the cache's LogWriter with log4j logging
updated all imports to be in the correct order for the new geode style guide

Signed-off-by: Brian Rowe <br...@pivotal.io>

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?

- [x] 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-3129

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

https://github.com/apache/geode/pull/621.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 #621


commit 9e4187fed638cfde5dc0a9a288e788a5b2cd9553
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-07-03T23:01:56Z

GEODE-3129 - Added error messages to protobuf protocol

added a new ErrorResponse type to ClientProtocol
removed success field from several RegionAPI response objects and 
refactored operation handlers to instead return ErrorResponses
made all op handlers take ClientProtocol.Requests and return 
ClientProtocol.Responses instead of operation specific types
moved the protobuf specific response building code from operation handlers 
to ProtobufResponseUtilities
moved the request building functions from MessageUtil (under Test) to 
ProtobufRequestUtilities
moved all utility classes to ...protocol.protobuf.utilities and added 
javadoc comments throughout
changed GetRegions to GetRegionNames, returns strings instead of Regions
replaced logging through the cache's LogWriter with log4j logging
updated all imports to be in the correct order for the new geode style guide

Signed-off-by: Brian Rowe <br...@pivotal.io>
Signed-off-by: Hitesh Khamesra <hitesh...@yahoo.com>




---
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 #617: GEODE-3129 - Added error messages to protobuf proto...

[WireBaron]

Github user WireBaron closed the pull request at:

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


---
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 #617: GEODE-3129 - Added error messages to protobuf proto...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3129 - Added error messages to protobuf protocol

added a new ErrorResponse type to ClientProtocol
removed success field from several RegionAPI response objects and 
refactored operation handlers to instead return ErrorResponses
made all op handlers take ClientProtocol.Requests and return 
ClientProtocol.Responses instead of operation specific types
moved the protobuf specific response building code from operation handlers 
to ProtobufResponseUtilities
moved the request building functions from MessageUtil (under Test) to 
ProtobufRequestUtilities
moved all utility classes to ...protocol.protobuf.utilities and added 
javadoc comments throughout
changed GetRegions to GetRegionNames, returns strings instead of Regions
replaced logging through the cache's LogWriter with log4j logging
updated all imports to be in the correct order for the new geode style guide

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?

- [x] 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-3129

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

https://github.com/apache/geode/pull/617.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 #617


commit 14c142d1853c87932ab58f66e2a0d54d3caf0652
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-07-03T23:01:56Z

GEODE-3129 - Added error messages to protobuf protocol

added a new ErrorResponse type to ClientProtocol
removed success field from several RegionAPI response objects and 
refactored operation handlers to instead return ErrorResponses
made all op handlers take ClientProtocol.Requests and return 
ClientProtocol.Responses instead of operation specific types
moved the protobuf specific response building code from operation handlers 
to ProtobufResponseUtilities
moved the request building functions from MessageUtil (under Test) to 
ProtobufRequestUtilities
moved all utility classes to ...protocol.protobuf.utilities and added 
javadoc comments throughout
changed GetRegions to GetRegionNames, returns strings instead of Regions
replaced logging through the cache's LogWriter with log4j logging
updated all imports to be in the correct order for the new geode style guide




---
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 #612: GEODE-3121: ensure that the protobuf protocol works...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3121: ensure that the protobuf protocol works over SSL

This just introduces a new test where we run the put/get integration test 
of the protocol module, but sets up a SSL cache and socket.

Signed-off-by: Brian Rowe <br...@pivotal.io>

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?

- [x] 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-3121

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

https://github.com/apache/geode/pull/612.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 #612


commit e0f1dce7872196ce194fea80c88ddcd58fe95b6f
Author: Hitesh Khamesra <hitesh...@yahoo.com>
Date:   2017-06-28T23:10:31Z

GEODE-3121: ensure that the protobuf protocol works over SSL

This just introduces a new test where we run the put/get integration test 
of the protocol module, but sets up a SSL cache and socket.

Signed-off-by: Brian Rowe <br...@pivotal.io>




---
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 #607: GEODE-3105: adding GetRegion handler for protobuf p...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3105: adding GetRegion handler for protobuf protocol

Added a handler which will catch incoming getRegion requests and will call 
into the cache's rootRegion and return the names of the region it finds.
Added unit test verifying hanlder behavior.
Added integration test verifying module correctness for getRegion.

Signed-off-by: Hitesh Khamesra <hitesh...@yahoo.com>

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?

- [x] 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-3105

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

https://github.com/apache/geode/pull/607.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 #607


commit f2d55db4e3eabfb51d13284c7cb0b0bacf5b2c05
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-06-27T23:15:53Z

GEODE-3105: adding GetRegion handler for protobuf protocol

Added a handler which will catch incoming getRegion requests and will call 
into the cache's rootRegion and return the names of the region it finds.
Added unit test verifying hanlder behavior.
Added integration test verifying module correctness for getRegion.

Signed-off-by: Brian Rowe <br...@pivotal.io>
Signed-off-by: Hitesh Khamesra <hitesh...@yahoo.com>




---
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 #605: GEODE-2996: incorporating review feedback and addin...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-2996: incorporating review feedback and adding integration test

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?

- [x] 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-2996

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

https://github.com/apache/geode/pull/605.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 #605


commit 3a2d2573151d260e8f030403a03cd47c88e366bf
Author: Alexander Murmann <amurm...@pivotal.io>
Date:   2017-06-27T00:56:06Z

GEODE-2996: incorporating review feedback and adding integration test

Addresses review feedback for GEODE-2996, mainly refactoring 
getOpertionHandler to handle failures like the putOperationHandler
Adding put operations to the RoundTripCacheConnectionJUnitTest, which is 
the integration test for the protobuf module
Removing service loading for protobuf operations and instead have the 
ProtobufStreamProcessor populate its OperationHandlerRegistry
Remove exception throwing from OperationHandler.process calls and remove 
TypeEncodingException

Signed-off-by: Brian Rowe <br...@pivotal.io>
Signed-off-by: Alexander Murmann <amurm...@pivotal.io>




---
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 #602: GEODE-3130: Refactoring AcceptorImpl

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3130: Refactoring AcceptorImpl

We extracted a switch statement in AcceptorImpl.handleNewClientConnection 
to a new method.

Signed-off-by: Alexander Murmann <amurm...@pivotal.io>

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?

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

- [ ] 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-3130

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

https://github.com/apache/geode/pull/602.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 #602


commit 01784841e4cfdb6750159aab286b734336df3467
Author: Brian Rowe <br...@pivotal.io>
Date:   2017-06-26T18:22:03Z

GEODE-3130: Refactoring AcceptorImpl

We extracted a switch statement in AcceptorImpl.handleNewClientConnection 
to a new method.

Signed-off-by: Alexander Murmann <amurm...@pivotal.io>




---
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 #593: GEODE-2995: Handle stream of ProtoBuf encoded messa...

[WireBaron]

Github user WireBaron closed the pull request at:

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


---
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 issue #567: GEODE-3051: Removing obsolete SSL handling in `AcceptorImp...

[WireBaron]

Github user WireBaron commented on the issue:

https://github.com/apache/geode/pull/567
  
This work is being handled as part of a larger refactor effort.


---
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 #567: GEODE-3051: Removing obsolete SSL handling in `Acce...

[WireBaron]

Github user WireBaron closed the pull request at:

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


---
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 #567: GEODE-3051: Removing obsolete SSL handling in `Acce...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3051: Removing obsolete SSL handling in `AcceptorImpl.accept` c…

…atch block

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?

- [x] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [x] 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, 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-3051

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

https://github.com/apache/geode/pull/567.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 #567


commit 9354cacfc6a8fa79746e8240730f6a209199bb89
Author: Galen O'Sullivan <gosulli...@pivotal.io>
Date:   2017-06-08T00:04:17Z

GEODE-3051: Removing obsolete SSL handling in `AcceptorImpl.accept` catch 
block




---
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.
---