[
https://issues.apache.org/jira/browse/GEODE-9804?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17446545#comment-17446545
]
ASF GitHub Bot commented on GEODE-9804:
---------------------------------------
pivotal-jbarrett commented on a change in pull request #894:
URL: https://github.com/apache/geode-native/pull/894#discussion_r753304896
##########
File path: cppcache/integration/benchmark/RegisterInterestBM.cpp
##########
@@ -0,0 +1,156 @@
+/*
+ * 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.
+ */
+
+#include <benchmark/benchmark.h>
+#include <framework/Cluster.h>
+#include <framework/Gfsh.h>
+
+#ifdef _MSC_VER
Review comment:
If we have to make more changes to this PR, let's pull this out of the
couple of headers it is in now and make a common 'logging.hpp' in the
integration framework
##########
File path: cppcache/integration/benchmark/RegisterInterestBM.cpp
##########
@@ -0,0 +1,156 @@
+/*
+ * 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.
+ */
+
+#include <benchmark/benchmark.h>
+#include <framework/Cluster.h>
+#include <framework/Gfsh.h>
+
+#ifdef _MSC_VER
+#pragma warning(push)
+#pragma warning(disable : 4596)
+#endif
+#include <boost/log/core.hpp>
+#include <boost/log/expressions.hpp>
+#include <boost/log/trivial.hpp>
+#ifdef _MSC_VER
+#pragma warning(pop)
+#endif
+
+#include <geode/Cache.hpp>
+#include <geode/CacheableString.hpp>
+#include <geode/PoolManager.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+namespace {
+
+using apache::geode::client::Cache;
+using apache::geode::client::CacheableString;
+using apache::geode::client::HashMapOfCacheable;
+using apache::geode::client::Region;
+using apache::geode::client::RegionShortcut;
+
+class RegisterInterestBM : public benchmark::Fixture {
+ public:
+ RegisterInterestBM() {
+ boost::log::core::get()->set_filter(boost::log::trivial::severity >=
+ boost::log::trivial::debug);
Review comment:
If the test is ready for prime time let's change the logging level to
warning.
##########
File path: cppcache/integration/test/RegisterKeysTest.cpp
##########
@@ -575,4 +605,227 @@ TEST(RegisterKeysTest, RegisterAnyWithProxyRegion) {
cache.close();
}
+apache::geode::client::Cache createCache() {
+ return apache::geode::client::CacheFactory()
+ .set("log-level", "debug")
+ .set("log-file", "c:/temp/RegisterKeysTest.log")
+ .set("statistic-sampling-enabled", "false")
+ .create();
+}
+
+std::shared_ptr<apache::geode::client::Pool> createPool(
+ Cluster& cluster, apache::geode::client::Cache& cache) {
+ auto poolFactory = cache.getPoolManager().createFactory();
+ cluster.applyLocators(poolFactory);
+ poolFactory.setSubscriptionEnabled(true); // Per the customer.
+ return poolFactory.create("default");
+}
+
+std::shared_ptr<apache::geode::client::Region> setupRegion(
+ apache::geode::client::Cache& cache,
+ const std::shared_ptr<apache::geode::client::Pool>& pool) {
+ auto region =
+ cache
+ .createRegionFactory(apache::geode::client::RegionShortcut::
+ CACHING_PROXY) // Per the customer.
+ .setPoolName(pool->getName())
+ .create("region");
+
+ return region;
+}
+
+TEST(RegisterKeysTest, DontReceiveValues) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+
+ cluster.start();
+
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("PARTITION")
+ .execute();
+
+ auto cache1 = createCache();
+ auto pool1 = createPool(cluster, cache1);
+ auto region1 = setupRegion(cache1, pool1);
+ auto attrMutator = region1->getAttributesMutator();
+
+ boost::latch allKeysUpdatedLatch{kNumKeys};
+ boost::latch allKeysInvalidLatch{kNumKeys};
+ auto listener = std::make_shared<MyCacheListener>(allKeysUpdatedLatch,
Review comment:
There is an issue with the lifespan of these latches not matching that
of the listener. Really the listener should construct and destroy these. Add
accessor methods to check them or just `wait_for` methods on the listener that
calls through to the latch's `wait_for` methods.
##########
File path: cppcache/integration/test/RegisterKeysTest.cpp
##########
@@ -575,4 +605,227 @@ TEST(RegisterKeysTest, RegisterAnyWithProxyRegion) {
cache.close();
}
+apache::geode::client::Cache createCache() {
+ return apache::geode::client::CacheFactory()
+ .set("log-level", "debug")
+ .set("log-file", "c:/temp/RegisterKeysTest.log")
+ .set("statistic-sampling-enabled", "false")
+ .create();
+}
+
+std::shared_ptr<apache::geode::client::Pool> createPool(
+ Cluster& cluster, apache::geode::client::Cache& cache) {
+ auto poolFactory = cache.getPoolManager().createFactory();
+ cluster.applyLocators(poolFactory);
+ poolFactory.setSubscriptionEnabled(true); // Per the customer.
+ return poolFactory.create("default");
+}
+
+std::shared_ptr<apache::geode::client::Region> setupRegion(
+ apache::geode::client::Cache& cache,
+ const std::shared_ptr<apache::geode::client::Pool>& pool) {
+ auto region =
+ cache
+ .createRegionFactory(apache::geode::client::RegionShortcut::
+ CACHING_PROXY) // Per the customer.
+ .setPoolName(pool->getName())
+ .create("region");
+
+ return region;
+}
+
+TEST(RegisterKeysTest, DontReceiveValues) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+
+ cluster.start();
+
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("PARTITION")
+ .execute();
+
+ auto cache1 = createCache();
+ auto pool1 = createPool(cluster, cache1);
+ auto region1 = setupRegion(cache1, pool1);
+ auto attrMutator = region1->getAttributesMutator();
+
+ boost::latch allKeysUpdatedLatch{kNumKeys};
+ boost::latch allKeysInvalidLatch{kNumKeys};
+ auto listener = std::make_shared<MyCacheListener>(allKeysUpdatedLatch,
+ allKeysInvalidLatch);
+
+ attrMutator->setCacheListener(listener);
+
+ auto cache2 = createCache();
+ auto pool2 = createPool(cluster, cache2);
+ auto region2 = setupRegion(cache2, pool2);
+
+ for (auto i = 0U; i < kNumKeys; i++) {
+ region2->put(CacheableInt32::create(i), CacheableInt32::create(i));
+ }
+
+ region1->registerAllKeys(false, false, false);
+
+ for (auto i = 0U; i < kNumKeys; i++) {
+ auto hasKey = region1->containsKey(CacheableInt32::create(i));
+ EXPECT_FALSE(hasKey);
+ }
+
+ for (auto i = 0U; i < kNumKeys; i++) {
+ auto value = region1->get(CacheableInt32::create(i));
+ }
+
+ allKeysInvalidLatch.reset(kNumKeys);
+ listener->reset();
+
+ for (auto i = 0U; i < kNumKeys; i++) {
+ region2->put(CacheableInt32::create(i), CacheableInt32::create(i + 1000));
+ }
+
+ EXPECT_EQ(boost::cv_status::no_timeout,
+ allKeysInvalidLatch.wait_for(boost::chrono::seconds(60)));
+
+ for (auto i = 0U; i < kNumKeys; i++) {
+ auto hasKey = region1->containsKey(CacheableInt32::create(i));
+ EXPECT_TRUE(hasKey);
+
+ auto hasValue = region1->containsValueForKey(CacheableInt32::create(i));
+ EXPECT_FALSE(hasValue);
+ }
+}
+
+TEST(RegisterKeysTest, ReceiveValuesLocalInvalidate) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+
+ cluster.start();
+
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("PARTITION")
+ .execute();
+
+ auto cache1 = createCache();
+ auto pool1 = createPool(cluster, cache1);
+ auto region1 = setupRegion(cache1, pool1);
+ auto attrMutator = region1->getAttributesMutator();
+
+ boost::latch allKeysUpdatedLatch{kNumKeys};
+ boost::latch allKeysInvalidLatch{kNumKeys};
+ auto listener = std::make_shared<MyCacheListener>(allKeysUpdatedLatch,
Review comment:
Same lifespan issue here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Both registerAllKeys and registerRegex always fetch initial entries.
> --------------------------------------------------------------------
>
> Key: GEODE-9804
> URL: https://issues.apache.org/jira/browse/GEODE-9804
> Project: Geode
> Issue Type: Bug
> Reporter: Jacob Barrett
> Priority: Major
> Labels: needsTriage, pull-request-available
>
> A inconsistency and bug in how the two regex interest methods configure the
> initial interest policy results in the both registerAllKeys and registerRegex
> fetching all initial entries despite the boolean parameter to get initial
> entries. Furthermore the misconfiguration results in none of the entries
> actually getting sent to the cache listener. The result is unnecessarily long
> registration times, network traffic and load on the servers. On servers with
> say millions of keys this can result in long GC pauses to unintentionally
> iterate over all those keys.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
