You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by up...@apache.org on 2018/08/02 21:19:08 UTC
[geode] 01/02: GEODE-5493: fix publication of client statistics
This is an automated email from the ASF dual-hosted git repository.
upthewaterspout pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
commit cd3783a6a0c5597ccd6799d3833e864b05719f15
Author: Helena A. Bales <hb...@pivotal.io>
AuthorDate: Fri Jul 27 16:31:07 2018 -0700
GEODE-5493: fix publication of client statistics
Fix publication of client statistics with security enabled. Remove dead
code related to retrieving old statistics that never worked. Add test
for fix.
Signed-off-by: Dan Smith <ds...@pivotal.io>
---
.../apache/geode/cache/client/internal/PutOp.java | 34 +----
.../cache/client/internal/ServerRegionProxy.java | 5 +-
.../geode/distributed/ConfigurationProperties.java | 2 +-
.../geode/internal/admin/ClientStatsManager.java | 60 +++-----
...ientStatisticsPublicationSecurityDUnitTest.java | 154 +++++++++++++++++++++
.../commands/DescribeClientCommandDUnitTest.java | 2 +-
6 files changed, 180 insertions(+), 77 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/PutOp.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/PutOp.java
index 8b0f0c4..e95c0e9 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/PutOp.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/PutOp.java
@@ -90,14 +90,14 @@ public class PutOp {
}
public static Object execute(ExecutablePool pool, String regionName, Object key, Object value,
- byte[] deltaBytes, EntryEventImpl event, Operation operation, boolean requireOldValue,
- Object expectedOldValue, Object callbackArg, boolean prSingleHopEnabled,
- boolean isMetaRegionPutOp) {
+ byte[] deltaBytes, EntryEventImpl event, Operation operation,
+ boolean requireOldValue,
+ Object expectedOldValue, Object callbackArg,
+ boolean prSingleHopEnabled) {
AbstractOp op = new PutOpImpl(regionName, key, value, deltaBytes, event, operation,
requireOldValue, expectedOldValue, callbackArg, false/* donot send full obj; send delta */,
prSingleHopEnabled);
- ((PutOpImpl) op).setMetaRegionPutOp(isMetaRegionPutOp);
return pool.execute(op);
}
@@ -151,8 +151,6 @@ public class PutOp {
private Object callbackArg;
- private boolean isMetaRegionPutOp;
-
private boolean prSingleHopEnabled;
private boolean requireOldValue;
@@ -393,26 +391,6 @@ public class PutOp {
}
@Override
- protected void sendMessage(Connection cnx) throws Exception {
- if (!this.isMetaRegionPutOp) {
- super.sendMessage(cnx);
- } else {
- getMessage().send(false);
- }
- }
-
- @Override
- protected void processSecureBytes(Connection cnx, Message message) throws Exception {
- super.processSecureBytes(cnx, message);
- }
-
- @Override
- protected boolean needsUserId() {
- boolean ret = this.isMetaRegionPutOp ? false : super.needsUserId();
- return ret;
- }
-
- @Override
protected boolean isErrorResponse(int msgType) {
return msgType == MessageType.PUT_DATA_ERROR;
}
@@ -471,10 +449,6 @@ public class PutOp {
return null;
}
}
-
- void setMetaRegionPutOp(boolean bool) {
- this.isMetaRegionPutOp = bool;
- }
}
}
diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/ServerRegionProxy.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/ServerRegionProxy.java
index a552a08..e3609f2 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/ServerRegionProxy.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/ServerRegionProxy.java
@@ -130,8 +130,7 @@ public class ServerRegionProxy extends ServerProxy implements ServerRegionDataAc
Object callbackArg, boolean isMetaRegionPutOp) {
if (this.region == null) {
return PutOp.execute(this.pool, this.regionName, key, value, deltaBytes, event,
- Operation.CREATE, false, null, callbackArg, this.pool.getPRSingleHopEnabled(),
- isMetaRegionPutOp);
+ Operation.CREATE, false, null, callbackArg, this.pool.getPRSingleHopEnabled());
} else {
return PutOp.execute(this.pool, this.region, key, value, deltaBytes, event, Operation.CREATE,
false, null, callbackArg, this.pool.getPRSingleHopEnabled());
@@ -150,7 +149,7 @@ public class ServerRegionProxy extends ServerProxy implements ServerRegionDataAc
if (this.region == null) {
return PutOp.execute(this.pool, this.regionName, key, value, deltaBytes, event, operation,
- requireOldValue, expectedOldValue, callbackArg, this.pool.getPRSingleHopEnabled(), false);
+ requireOldValue, expectedOldValue, callbackArg, this.pool.getPRSingleHopEnabled());
} else {
return PutOp.execute(this.pool, this.region, key, value, deltaBytes, event, operation,
requireOldValue, expectedOldValue, callbackArg, this.pool.getPRSingleHopEnabled());
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java b/geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
index 858b223..089a006 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
@@ -1175,7 +1175,7 @@ public interface ConfigurationProperties {
/**
* The static String definition of the <i>"log-file"</i> property <a name="log-file"/a>
* </p>
- * <U>Description</U>: Name of the file to write logging messages to. If the file name if ""
+ * <U>Description</U>: Name of the file to write logging messages to. If the file name is ""
* (default) then messages are written to standard out.
* </p>
* <U>Default</U>: ""
diff --git a/geode-core/src/main/java/org/apache/geode/internal/admin/ClientStatsManager.java b/geode-core/src/main/java/org/apache/geode/internal/admin/ClientStatsManager.java
index c333906..2ea963c 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/admin/ClientStatsManager.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/admin/ClientStatsManager.java
@@ -16,25 +16,24 @@ package org.apache.geode.internal.admin;
import java.util.Date;
import java.util.Map;
-import java.util.Map.Entry;
import org.apache.commons.lang.exception.ExceptionUtils;
+import org.apache.logging.log4j.Logger;
import org.apache.geode.Statistics;
import org.apache.geode.StatisticsType;
import org.apache.geode.cache.CacheWriterException;
-import org.apache.geode.cache.Region;
import org.apache.geode.cache.client.internal.PoolImpl;
import org.apache.geode.cache.client.internal.ServerRegionProxy;
import org.apache.geode.distributed.DistributedSystemDisconnectedException;
import org.apache.geode.distributed.internal.InternalDistributedSystem;
-import org.apache.geode.i18n.LogWriterI18n;
import org.apache.geode.internal.admin.remote.ClientHealthStats;
import org.apache.geode.internal.cache.EntryEventImpl;
import org.apache.geode.internal.cache.EventID;
import org.apache.geode.internal.cache.GemFireCacheImpl;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.i18n.LocalizedStrings;
+import org.apache.geode.internal.logging.LogService;
import org.apache.geode.internal.offheap.annotations.Released;
/**
@@ -59,6 +58,8 @@ public class ClientStatsManager {
*/
private static Statistics vmStats = null;
+ private static final Logger logger = LogService.getLogger();
+
/**
* This method publishes the client stats using the admin region.
*
@@ -69,9 +70,9 @@ public class ClientStatsManager {
if (!initializeStatistics(currentCache)) {
return; // handles null case too
}
- LogWriterI18n logger = currentCache.getLoggerI18n();
- if (logger.fineEnabled())
- logger.fine("Entering ClientStatsManager#publishClientStats...");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Entering ClientStatsManager#publishClientStats...");
+ }
ClientHealthStats stats = getClientHealthStats(currentCache, pool);
@@ -96,7 +97,7 @@ public class ClientStatsManager {
pool.getCancelCriterion().checkCancelInProgress(cwx);
currentCache.getCancelCriterion().checkCancelInProgress(cwx);
// TODO: Need to analyze these exception scenarios.
- logger.warning(
+ logger.warn(
LocalizedStrings.ClientStatsManager_FAILED_TO_SEND_CLIENT_HEALTH_STATS_TO_CACHESERVER,
cwx);
} catch (Exception e) {
@@ -105,8 +106,8 @@ public class ClientStatsManager {
logger.info(LocalizedStrings.ClientStatsManager_FAILED_TO_PUBLISH_CLIENT_STATISTICS, e);
}
- if (logger.fineEnabled()) {
- logger.fine("Exiting ClientStatsManager#publishClientStats.");
+ if (logger.isDebugEnabled()) {
+ logger.debug("Exiting ClientStatsManager#publishClientStats.");
}
}
@@ -125,7 +126,7 @@ public class ClientStatsManager {
if (currentCache == null) {
return false;
}
- LogWriterI18n logger = currentCache.getLoggerI18n();
+
InternalDistributedSystem ds = (InternalDistributedSystem) currentCache.getDistributedSystem();
if (currentCache.isClosed()) {
return false;
@@ -135,7 +136,7 @@ public class ClientStatsManager {
lastInitializedCache = currentCache;
if (restart) {
- if (logger.infoEnabled()) {
+ if (logger.isInfoEnabled()) {
logger.info(
LocalizedStrings.ClientStatsManager_CLIENTSTATSMANAGER_INTIALIZING_THE_STATISTICS);
}
@@ -165,14 +166,14 @@ public class ClientStatsManager {
// Validate that cache has changed before logging the warning, thus logging it once per cache
if (cachePerfStats == null && restart) {
- logger.warning(LocalizedStrings.ClientStatsManager_CLIENTSTATSMANAGER_0_ARE_NOT_AVAILABLE,
- "CachePerfStats");
+ logger.warn(LocalizedStrings.ClientStatsManager_CLIENTSTATSMANAGER_0_ARE_NOT_AVAILABLE
+ .toLocalizedString("CachePerfStats"));
}
// Validate that cache has changed before logging the warning, thus logging it once per cache
if (vmStats == null && restart) {
- logger.warning(LocalizedStrings.ClientStatsManager_CLIENTSTATSMANAGER_0_ARE_NOT_AVAILABLE,
- "VMStats");
+ logger.warn(LocalizedStrings.ClientStatsManager_CLIENTSTATSMANAGER_0_ARE_NOT_AVAILABLE
+ .toLocalizedString("VMStats"));
}
return true;
@@ -189,7 +190,6 @@ public class ClientStatsManager {
return null;
}
ClientHealthStats stats = new ClientHealthStats();
- LogWriterI18n logger = currentCache.getLoggerI18n();
int gets = -1;
int puts = -1;
@@ -226,36 +226,12 @@ public class ClientStatsManager {
String poolStatsStr = "MinConnections=" + pool.getMinConnections() + ";MaxConnections="
+ pool.getMaxConnections() + ";Redundancy=" + pool.getSubscriptionRedundancy() + ";CQS="
+ pool.getQueryService().getCqs().length;
- logger.info(LocalizedStrings.DEBUG,
- "ClientHealthStats for poolName " + poolName + " poolStatsStr=" + poolStatsStr);
+ logger.debug("ClientHealthStats for poolName " + poolName + " poolStatsStr=" + poolStatsStr);
newPoolStats.put(poolName, poolStatsStr);
- // consider old stats
- Region clientHealthMonitoringRegion = ClientHealthMonitoringRegion.getInstance(currentCache);
-
- if (clientHealthMonitoringRegion != null) {
- InternalDistributedSystem ds =
- (InternalDistributedSystem) currentCache.getDistributedSystem();
- ClientHealthStats oldStats =
- (ClientHealthStats) clientHealthMonitoringRegion.get(ds.getMemberId());
- logger.info(LocalizedStrings.DEBUG, "getClientHealthStats got oldStats " + oldStats);
- if (oldStats != null) {
- Map<String, String> oldPoolStats = oldStats.getPoolStats();
- logger.info(LocalizedStrings.DEBUG,
- "getClientHealthStats got oldPoolStats " + oldPoolStats);
- if (oldPoolStats != null) {
- for (Entry<String, String> entry : oldPoolStats.entrySet()) {
- if (!poolName.equals(entry.getKey())) {
- stats.getPoolStats().put(entry.getKey(), entry.getValue());
- }
- }
- }
- }
- }
-
} catch (Exception e) {
- logger.fine("Exception in getting pool stats in getClientHealthStats "
+ logger.debug("Exception in getting pool stats in getClientHealthStats "
+ ExceptionUtils.getStackTrace(e));
}
diff --git a/geode-cq/src/distributedTest/java/org/apache/geode/cache/ClientStatisticsPublicationSecurityDUnitTest.java b/geode-cq/src/distributedTest/java/org/apache/geode/cache/ClientStatisticsPublicationSecurityDUnitTest.java
new file mode 100644
index 0000000..2a66b42
--- /dev/null
+++ b/geode-cq/src/distributedTest/java/org/apache/geode/cache/ClientStatisticsPublicationSecurityDUnitTest.java
@@ -0,0 +1,154 @@
+/*
+ * 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.cache;
+
+import static org.apache.geode.cache.client.ClientRegionShortcut.CACHING_PROXY;
+import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_CLIENT_AUTH_INIT;
+import static org.apache.geode.test.dunit.Invoke.invokeInEveryVM;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Properties;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.logging.log4j.Logger;
+import org.awaitility.Awaitility;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.Pool;
+import org.apache.geode.cache.client.PoolFactory;
+import org.apache.geode.cache.client.PoolManager;
+import org.apache.geode.cache.client.internal.PoolImpl;
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.admin.ClientStatsManager;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.internal.net.SocketCreator;
+import org.apache.geode.management.CacheServerMXBean;
+import org.apache.geode.management.ClientHealthStatus;
+import org.apache.geode.management.ManagementService;
+import org.apache.geode.management.internal.SystemManagementService;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.security.templates.UserPasswordAuthInit;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.categories.ClientServerTest;
+import org.apache.geode.test.junit.rules.GfshCommandRule;
+
+@Category({ClientServerTest.class})
+public class ClientStatisticsPublicationSecurityDUnitTest {
+ private static MemberVM locator, server;
+
+ @Rule
+ public final GfshCommandRule gfsh = new GfshCommandRule();
+
+ @Rule
+ public final ClusterStartupRule cluster = new ClusterStartupRule();
+
+ private static final Logger log = LogService.getLogger();
+
+ @Before
+ public void before() {
+ invokeInEveryVM(() -> {
+ // TODO: Remove this once GEODE-5157 is fixed
+ SocketCreator.use_client_host_name = false;
+ });
+
+ Properties locatorProps = new Properties();
+ locatorProps.setProperty(CliStrings.START_LOCATOR__MEMBER_NAME, "locator1");
+ locatorProps.setProperty("security-manager", "org.apache.geode.examples.SimpleSecurityManager");
+ locator = cluster.startLocatorVM(0, locatorProps);
+
+ Properties serverProps = new Properties();
+ serverProps.setProperty(CliStrings.START_SERVER__NAME, "server1");
+ serverProps.setProperty(CliStrings.START_SERVER__LOCATORS,
+ "localhost[" + locator.getPort() + "]");
+ serverProps.setProperty("security-manager", "org.apache.geode.examples.SimpleSecurityManager");
+ serverProps.setProperty("security-username", "cluster");
+ serverProps.setProperty("security-password", "cluster");
+ server = cluster.startServerVM(1, serverProps);
+ server.invoke(() -> {
+ InternalCache cache = InternalDistributedSystem.getConnectedInstance().getCache();
+
+ cache.createRegionFactory(RegionShortcut.REPLICATE).create("regionName");
+ });
+ locator.waitUntilRegionIsReadyOnExactlyThisManyServers("/regionName", 1);
+ }
+
+ @Test
+ public void testClientCanPublishStatisticsWithSecurity() throws Exception {
+ final String regionName = "regionName";
+
+ // TODO: Remove this once GEODE-5157 is fixed
+ SocketCreator.use_client_host_name = false;
+
+ ClientCache client = new ClientCacheFactory()
+ .set(ConfigurationProperties.LOG_LEVEL, "fine")
+ .set(ConfigurationProperties.LOG_FILE, "")
+ .set("security-manager", "org.apache.geode.examples.SimpleSecurityManager")
+ .set("security-username", "data")
+ .set("security-password", "data")
+ .set(SECURITY_CLIENT_AUTH_INIT, UserPasswordAuthInit.class.getName())
+ .create();
+ PoolFactory poolFactory = PoolManager.createFactory();
+ poolFactory.addServer("localhost", server.getPort());
+ poolFactory.setStatisticInterval(1000);
+ poolFactory.setSubscriptionEnabled(true);
+ Pool pool = poolFactory.create("poolName");
+
+
+ Region region =
+ client.createClientRegionFactory(CACHING_PROXY).setPoolName("poolName").create(regionName);
+
+ checkClientHealthStatus(0);
+
+ region.put("key", "value");
+
+ ClientStatsManager.publishClientStats((PoolImpl) pool);
+
+ checkClientHealthStatus(1, "poolName");
+
+ client.close(false);
+ }
+
+ private void checkClientHealthStatus(int expectedNumPuts,
+ String... expectedPoolStatKeys) {
+ final int serverPort = server.getPort();
+ server.invoke(() -> {
+ Awaitility.waitAtMost(1, TimeUnit.MINUTES).until(() -> {
+ Cache cache = ClusterStartupRule.getCache();
+ SystemManagementService service =
+ (SystemManagementService) ManagementService.getExistingManagementService(cache);
+ CacheServerMXBean serviceMBean = service.getJMXAdapter().getClientServiceMXBean(serverPort);
+ try {
+ String clientId = serviceMBean.getClientIds()[0];
+ ClientHealthStatus status = serviceMBean.showClientStats(clientId);
+ assertThat(status.getNumOfPuts()).isEqualTo(expectedNumPuts);
+ assertThat(status.getPoolStats().keySet())
+ .containsExactlyInAnyOrder(expectedPoolStatKeys);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+
+ });
+ });
+ }
+}
diff --git a/geode-cq/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/DescribeClientCommandDUnitTest.java b/geode-cq/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/DescribeClientCommandDUnitTest.java
index fb8d45f..fdf9a6c 100644
--- a/geode-cq/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/DescribeClientCommandDUnitTest.java
+++ b/geode-cq/src/distributedTest/java/org/apache/geode/management/internal/cli/commands/DescribeClientCommandDUnitTest.java
@@ -151,7 +151,7 @@ public class DescribeClientCommandDUnitTest {
}
assertThat(data.get(CliStrings.DESCRIBE_CLIENT_COLUMN_PUTS)).isEqualTo("2");
- assertThat(data.get(CliStrings.DESCRIBE_CLIENT_COLUMN_LISTENER_CALLS)).isEqualTo("1");
+ assertThat(data.get(CliStrings.DESCRIBE_CLIENT_COLUMN_LISTENER_CALLS)).isEqualTo("0");
assertThat(data.get(CliStrings.DESCRIBE_CLIENT_COLUMN_DURABLE)).isEqualTo("No");
assertThat(Integer.parseInt(data.get(CliStrings.DESCRIBE_CLIENT_COLUMN_THREADS)))
.isGreaterThan(0);