pdxcodemonkey commented on a change in pull request #892:
URL: https://github.com/apache/geode-native/pull/892#discussion_r748733902



##########
File path: cppcache/integration/benchmark/RegionBM.cpp
##########
@@ -19,17 +19,16 @@
 #include <framework/Cluster.h>
 #include <framework/Gfsh.h>
 
-// Disable warning for "extra qualifications" here.  One of the boost log
-// headers triggers this warning.  Note: use of disable pragma here is
-// intentional - attempts to use push/pop as you ordinarily should just
-// yielded a gripe from the MS tools that "warning number '4596' is not a
-// valid compiler warning". re-enabling the warning after the include
-// fails in the same way, so just leave it disabled for the rest of the

Review comment:
       I take it from the change that this comment is no longer applicable?  
Just double-checking....

##########
File path: cppcache/integration/benchmark/RegisterInterestBM.cpp
##########
@@ -0,0 +1,162 @@
+/*
+ * 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);
+
+    BOOST_LOG_TRIVIAL(info) << "constructed";
+  }
+
+  ~RegisterInterestBM() noexcept override {
+    BOOST_LOG_TRIVIAL(info) << "destructed";
+  }
+
+  using benchmark::Fixture::SetUp;
+  void SetUp(benchmark::State& state) override {
+    BOOST_LOG_TRIVIAL(info) << "starting cluster";
+    cluster = std::unique_ptr<Cluster>(
+        new Cluster(::Name{name_}, LocatorCount{1}, ServerCount{4}));
+    cluster->start();
+    cluster->getGfsh()
+        .create()
+        .region()
+        .withName("region")
+        .withType("PARTITION")
+        .execute();
+
+    cache = std::unique_ptr<Cache>(new Cache(cluster->createCache(
+        {{"log-level", "finer"}}, Cluster::SubscriptionState::Enabled)));
+    region = cache->createRegionFactory(RegionShortcut::PROXY)
+                 .setPoolName("default")
+                 .setCachingEnabled(true)
+                 .create("region");
+
+    BOOST_LOG_TRIVIAL(info)
+        << "filling region with " << state.range(0) << " keys";
+    HashMapOfCacheable map;
+    const auto batchSize = 10000;
+    map.reserve(batchSize);
+    for (auto i = 0; i < state.range(0); ++i) {
+      map.emplace(
+          std::make_shared<CacheableString>("key" + std::to_string(i)),
+          std::make_shared<CacheableString>("value" + std::to_string(i)));
+      if (0 == i % batchSize) {
+        region->putAll(map);
+        map.clear();
+      }
+    }
+    if (!map.empty()) {
+      region->putAll(map);
+      map.clear();
+    }
+    BOOST_LOG_TRIVIAL(info) << "region ready";
+  }
+
+  using benchmark::Fixture::TearDown;
+  void TearDown(benchmark::State&) override {
+    BOOST_LOG_TRIVIAL(info) << "stopping cluster";
+    region = nullptr;
+    cache = nullptr;
+    cluster = nullptr;
+  }
+
+ protected:
+  void SetName(const char* name) {
+    name_ = name;
+
+    Benchmark::SetName(name);
+  }
+
+  void unregisterInterestAllKeys(benchmark::State& state) {
+    state.PauseTiming();
+    region->unregisterAllKeys();
+    state.ResumeTiming();
+  }
+
+  std::unique_ptr<Cluster> cluster;
+  std::unique_ptr<Cache> cache;
+  std::shared_ptr<Region> region;
+
+ private:
+  std::string name_;
+};
+
+BENCHMARK_DEFINE_F(RegisterInterestBM, registerInterestAllKeys)
+(benchmark::State& state) {
+  for (auto _ : state) {
+    region->registerAllKeys();
+    unregisterInterestAllKeys(state);
+  }
+}
+BENCHMARK_REGISTER_F(RegisterInterestBM, registerInterestAllKeys)
+    ->Unit(benchmark::kMillisecond)
+    ->Repetitions(1)
+    ->Iterations(10)
+    ->Arg(1000000)
+    // ->Arg(5000000)

Review comment:
       Why leave in commented lines here?  Absent a compelling reason, please 
remove.

##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -421,111 +424,64 @@ void ThinClientRegion::unregisterKeys(
         "Unregister keys "
         "keys vector is empty");
   }
-  GfErrType err = unregisterKeysNoThrow(keys);
+  const auto err = unregisterKeysNoThrow(keys);
   throwExceptionIfError("Region::unregisterKeys", err);
 }
 
-void ThinClientRegion::registerAllKeys(bool isDurable, bool getInitialValues,
-                                       bool receiveValues) {
-  auto pool = 
m_cacheImpl->getPoolManager().find(getAttributes().getPoolName());
-  if (pool != nullptr) {
+void ThinClientRegion::registerAllKeys(const bool isDurable,
+                                       const bool getInitialValues,
+                                       const bool receiveValues) {
+  registerRegex(kAllKeysRegex, isDurable, getInitialValues, receiveValues);
+}
+
+void ThinClientRegion::registerRegex(const std::string& regex,
+                                     const bool isDurable,
+                                     const bool getInitialValues,
+                                     const bool receiveValues) {
+  if (auto pool =
+          m_cacheImpl->getPoolManager().find(getAttributes().getPoolName())) {

Review comment:
       Same as above - what does a null pool mean here?

##########
File path: cppcache/src/InterestResultPolicy.hpp
##########
@@ -20,34 +15,28 @@
  * limitations under the License.
  */
 
-/**
- * @file
- */
-#include <geode/internal/geode_globals.hpp>
+#pragma once
+
+#ifndef GEODE_INTERESTRESULTPOLICY_H_
+#define GEODE_INTERESTRESULTPOLICY_H_
+
+#include <cstdint>
 
 namespace apache {
 namespace geode {
 namespace client {
+
 /**
- * @class InterestResultPolicy InterestResultPolicy.hpp
  * Policy class for interest result.
+ *
+ * Note: Special DataSeralizableFixedId(37) type.

Review comment:
       What does this mean?  I'm missing context here...

##########
File path: cppcache/src/InterestResultPolicy.hpp
##########
@@ -20,34 +15,28 @@
  * limitations under the License.
  */
 
-/**
- * @file
- */
-#include <geode/internal/geode_globals.hpp>
+#pragma once
+
+#ifndef GEODE_INTERESTRESULTPOLICY_H_
+#define GEODE_INTERESTRESULTPOLICY_H_
+
+#include <cstdint>
 
 namespace apache {
 namespace geode {
 namespace client {
+
 /**
- * @class InterestResultPolicy InterestResultPolicy.hpp
  * Policy class for interest result.
+ *
+ * Note: Special DataSeralizableFixedId(37) type.
  */
-class InterestResultPolicy {
-  // public static methods
- public:
-  static char nextOrdinal;
-
-  static InterestResultPolicy NONE;
-  static InterestResultPolicy KEYS;
-  static InterestResultPolicy KEYS_VALUES;
-
-  char ordinal;
-
-  char getOrdinal() { return ordinal; }
-
- private:
-  InterestResultPolicy() { ordinal = nextOrdinal++; }
+enum class InterestResultPolicy : int8_t {

Review comment:
       Nice change!  Good to be rid of at least one more small gremlin in the 
code base.

##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -2081,9 +2075,15 @@ 
TcrMessageRegisterInterestList::TcrMessageRegisterInterestList(
   m_regionName =
       region == nullptr ? "INVALID_REGION_NAME" : region->getFullPath();
   m_region = region;
-  m_timeout = DEFAULT_TIMEOUT;

Review comment:
       Why was this removed?  Is there no timeout associated with this message?

##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -339,10 +339,11 @@ void ThinClientRegion::initTCR() {
 }
 
 void ThinClientRegion::registerKeys(
-    const std::vector<std::shared_ptr<CacheableKey>>& keys, bool isDurable,
-    bool getInitialValues, bool receiveValues) {
-  auto pool = 
m_cacheImpl->getPoolManager().find(getAttributes().getPoolName());
-  if (pool != nullptr) {
+    const std::vector<std::shared_ptr<CacheableKey>>& keys,
+    const bool isDurable, const bool getInitialValues,
+    const bool receiveValues) {
+  if (auto pool =
+          m_cacheImpl->getPoolManager().find(getAttributes().getPoolName())) {

Review comment:
       Is it possible to get here with a null pool?  Should we be throwing an 
error, or at least logging one, in that case?

##########
File path: cppcache/src/TcrMessage.hpp
##########
@@ -607,26 +605,27 @@ class TcrMessageCreateRegion : public TcrMessage {
   ~TcrMessageCreateRegion() override = default;
 };
 
-class TcrMessageRegisterInterest : public TcrMessage {
+class TcrMessageRegisterInterestRegex : public TcrMessage {
  public:
-  TcrMessageRegisterInterest(
-      DataOutput* dataOutput, const std::string& str1, const std::string& str2,
+  TcrMessageRegisterInterestRegex(
+      DataOutput* dataOutput, const std::string& regionName,
+      const std::string& regex,
       InterestResultPolicy interestPolicy = InterestResultPolicy::NONE,
       bool isDurable = false, bool isCachingEnabled = false,
       bool receiveValues = true, ThinClientBaseDM* connectionDM = nullptr);
 
-  ~TcrMessageRegisterInterest() override = default;
+  ~TcrMessageRegisterInterestRegex() override = default;
 };
 
-class TcrMessageUnregisterInterest : public TcrMessage {
+class TcrMessageUnregisterInterestRegex : public TcrMessage {
  public:
-  TcrMessageUnregisterInterest(
-      DataOutput* dataOutput, const std::string& str1, const std::string& str2,
-      InterestResultPolicy interestPolicy = InterestResultPolicy::NONE,
-      bool isDurable = false, bool receiveValues = true,
-      ThinClientBaseDM* connectionDM = nullptr);
+  TcrMessageUnregisterInterestRegex(DataOutput* dataOutput,

Review comment:
       This kind of looks like a `clang-format` glitch?!  Wonder why it 
reformatted this ctor, but not the one above...

##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -2181,64 +2184,82 @@ TcrMessageCreateRegion::TcrMessageCreateRegion(
   m_regionName = str2;
 }
 
-TcrMessageRegisterInterest::TcrMessageRegisterInterest(
-    DataOutput* dataOutput, const std::string& str1, const std::string& str2,
-    InterestResultPolicy interestPolicy, bool isDurable, bool isCachingEnabled,
-    bool receiveValues, ThinClientBaseDM* connectionDM) {
+TcrMessageRegisterInterestRegex::TcrMessageRegisterInterestRegex(
+    DataOutput* dataOutput, const std::string& regionName,
+    const std::string& regex, InterestResultPolicy interestPolicy,
+    bool isDurable, bool isCachingEnabled, bool receiveValues,
+    ThinClientBaseDM* connectionDM) {
   m_request.reset(dataOutput);
   m_msgType = TcrMessage::REGISTER_INTEREST;
   m_tcdm = connectionDM;
   m_isDurable = isDurable;
   m_receiveValues = receiveValues;
+  m_regionName = regionName;
+  m_regex = regex;
+  m_interestPolicy = interestPolicy;
 
-  uint32_t numOfParts = 7;
+  writeHeader(m_msgType, 7);
 
-  writeHeader(m_msgType, numOfParts);
+  // part 0
+  writeRegionPart(regionName);
+
+  // part 1
+  writeIntPart(kREGULAR_EXPRESSION);  // InterestType
+
+  // part 2
+  writeInterestResultPolicyPart(interestPolicy);
 
-  writeRegionPart(str1);                          // region name
-  writeIntPart(kREGULAR_EXPRESSION);              // InterestType
-  writeInterestResultPolicyPart(interestPolicy);  // InterestResultPolicy
+  // part 3
   writeBytePart(isDurable ? 1 : 0);
-  writeRegionPart(str2);  // regexp string
+
+  // part 4
+  writeRegionPart(regex);  // regexp string
 
   int8_t bytes[2];
+
+  // part 5
   std::shared_ptr<CacheableBytes> byteArr = nullptr;
   bytes[0] = receiveValues ? 0 : 1;
   byteArr = CacheableBytes::create(std::vector<int8_t>(bytes, bytes + 1));
   writeObjectPart(byteArr);
+
+  // part 6
   bytes[0] = isCachingEnabled ? 1 : 0;  // region data policy
   bytes[1] = 0;                         // serializevalues
   byteArr = CacheableBytes::create(std::vector<int8_t>(bytes, bytes + 2));
   writeObjectPart(byteArr);
 
   writeMessageLength();
-  m_regionName = str1;

Review comment:
       lol, I suppose we were just storing these here because "reasons"?

##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -327,7 +321,7 @@ void 
TcrMessage::writeInterestResultPolicyPart(InterestResultPolicy policy) {
   m_request->write(static_cast<int8_t>(1));      // isObject
   m_request->write(static_cast<int8_t>(DSCode::FixedIDByte));
   m_request->write(static_cast<int8_t>(DSCode::InterestResultPolicy));
-  m_request->write(static_cast<int8_t>(policy.getOrdinal()));

Review comment:
       No comment on the replacement code, I'm just trying to wrap my head 
around what the heck the original code did.  So we used to write 2 bytes, the 
first the actual policy and the 2nd, essentially, the count of how many times 
getOrdinal() had been called?  The original code just seems so wildly incorrect 
- I can't tell for sure what it was doing, but there's no way it could have 
been right....

##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -2181,64 +2184,82 @@ TcrMessageCreateRegion::TcrMessageCreateRegion(
   m_regionName = str2;
 }
 
-TcrMessageRegisterInterest::TcrMessageRegisterInterest(
-    DataOutput* dataOutput, const std::string& str1, const std::string& str2,
-    InterestResultPolicy interestPolicy, bool isDurable, bool isCachingEnabled,
-    bool receiveValues, ThinClientBaseDM* connectionDM) {
+TcrMessageRegisterInterestRegex::TcrMessageRegisterInterestRegex(
+    DataOutput* dataOutput, const std::string& regionName,
+    const std::string& regex, InterestResultPolicy interestPolicy,
+    bool isDurable, bool isCachingEnabled, bool receiveValues,
+    ThinClientBaseDM* connectionDM) {
   m_request.reset(dataOutput);
   m_msgType = TcrMessage::REGISTER_INTEREST;
   m_tcdm = connectionDM;
   m_isDurable = isDurable;
   m_receiveValues = receiveValues;
+  m_regionName = regionName;
+  m_regex = regex;
+  m_interestPolicy = interestPolicy;
 
-  uint32_t numOfParts = 7;
+  writeHeader(m_msgType, 7);

Review comment:
       A named constant might add some clarity here.  Casual readers have no 
idea what's going on with "parts" of a Geode message.

##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2208,45 +2149,40 @@ GfErrType ThinClientRegion::registerKeysNoThrow(
   RegionGlobalLocks acquireLocksRedundancy(this, false);
   RegionGlobalLocks acquireLocksFailover(this);
   CHECK_DESTROY_PENDING_NOTHROW(shared_lock);
-  GfErrType err = GF_NOERR;
+  auto err = GF_NOERR;
 
   std::lock_guard<decltype(m_keysLock)> keysGuard(m_keysLock);
   if (keys.empty()) {
     return err;
   }
 
-  TcrMessageReply replyLocal(true, m_tcrdm.get());
-  bool needToCreateRC = true;
-  if (reply == nullptr) {
-    reply = &replyLocal;
-  } else {
-    needToCreateRC = false;
-  }
-
-  LOGDEBUG("ThinClientRegion::registerKeysNoThrow : interestpolicy is %d",
-           interestPolicy.ordinal);
+  LOGDEBUG("ThinClientRegion::registerKeysNoThrow : interestPolicy is %d",
+           interestPolicy);
 
   TcrMessageRegisterInterestList request(
       new DataOutput(m_cacheImpl->createDataOutput()), this, keys, isDurable,
       getAttributes().getCachingEnabled(), receiveValues, interestPolicy,
       m_tcrdm.get());
+
   std::recursive_mutex responseLock;
-  TcrChunkedResult* resultCollector = nullptr;
-  if (interestPolicy.ordinal == InterestResultPolicy::KEYS_VALUES.ordinal) {
-    auto values = std::make_shared<HashMapOfCacheable>();
-    auto exceptions = std::make_shared<HashMapOfException>();
-    MapOfUpdateCounters trackers;
-    int32_t destroyTracker = 1;
-    if (needToCreateRC) {
-      resultCollector = (new ChunkedGetAllResponse(
-          request, this, &keys, values, exceptions, nullptr, trackers,
-          destroyTracker, true, responseLock));
-      reply->setChunkedResultHandler(resultCollector);
-    }
-  } else {
-    if (needToCreateRC) {
-      resultCollector = (new ChunkedInterestResponse(request, nullptr, 
*reply));
-      reply->setChunkedResultHandler(resultCollector);
+  std::unique_ptr<TcrChunkedResult> resultCollector;
+  TcrMessageReply replyLocal(true, m_tcrdm.get());
+  if (!reply) {
+    reply = &replyLocal;
+    if (interestPolicy == InterestResultPolicy::KEYS_VALUES) {
+      auto values = std::make_shared<HashMapOfCacheable>();
+      auto exceptions = std::make_shared<HashMapOfException>();

Review comment:
       Why is this map here?  Can we push it down into the 
`ChunkedGetAllResponse` ctor?

##########
File path: cppcache/src/TcrMessage.cpp
##########
@@ -2181,64 +2184,82 @@ TcrMessageCreateRegion::TcrMessageCreateRegion(
   m_regionName = str2;
 }
 
-TcrMessageRegisterInterest::TcrMessageRegisterInterest(
-    DataOutput* dataOutput, const std::string& str1, const std::string& str2,
-    InterestResultPolicy interestPolicy, bool isDurable, bool isCachingEnabled,
-    bool receiveValues, ThinClientBaseDM* connectionDM) {
+TcrMessageRegisterInterestRegex::TcrMessageRegisterInterestRegex(
+    DataOutput* dataOutput, const std::string& regionName,
+    const std::string& regex, InterestResultPolicy interestPolicy,
+    bool isDurable, bool isCachingEnabled, bool receiveValues,
+    ThinClientBaseDM* connectionDM) {
   m_request.reset(dataOutput);
   m_msgType = TcrMessage::REGISTER_INTEREST;
   m_tcdm = connectionDM;
   m_isDurable = isDurable;
   m_receiveValues = receiveValues;
+  m_regionName = regionName;
+  m_regex = regex;
+  m_interestPolicy = interestPolicy;
 
-  uint32_t numOfParts = 7;
+  writeHeader(m_msgType, 7);
 
-  writeHeader(m_msgType, numOfParts);
+  // part 0
+  writeRegionPart(regionName);
+
+  // part 1
+  writeIntPart(kREGULAR_EXPRESSION);  // InterestType
+
+  // part 2
+  writeInterestResultPolicyPart(interestPolicy);
 
-  writeRegionPart(str1);                          // region name
-  writeIntPart(kREGULAR_EXPRESSION);              // InterestType
-  writeInterestResultPolicyPart(interestPolicy);  // InterestResultPolicy
+  // part 3
   writeBytePart(isDurable ? 1 : 0);
-  writeRegionPart(str2);  // regexp string
+
+  // part 4
+  writeRegionPart(regex);  // regexp string

Review comment:
       Misleading  name here.  "Region part" is (or should be) the name of a 
region, this is writing, essentially, a "regex part" relying on the fact that 
both are just a part containing a string.

##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -2384,64 +2317,51 @@ GfErrType ThinClientRegion::registerRegexNoThrow(
     }
   }
 
-  ChunkedInterestResponse* resultCollector = nullptr;
-  ChunkedGetAllResponse* getAllResultCollector = nullptr;
-  if (reply != nullptr) {
-    // need to check
-    resultCollector = dynamic_cast<ChunkedInterestResponse*>(
-        reply->getChunkedResultHandler());
-    if (resultCollector != nullptr) {
-      resultKeys = resultCollector->getResultKeys();
-    } else {
-      getAllResultCollector = dynamic_cast<ChunkedGetAllResponse*>(
-          reply->getChunkedResultHandler());
-      resultKeys = getAllResultCollector->getResultKeys();
-    }
-  }
+  LOGDEBUG("ThinClientRegion::registerRegexNoThrow : interestPolicy is %d",
+           interestPolicy);
 
-  bool isRCCreatedLocally = false;
-  LOGDEBUG("ThinClientRegion::registerRegexNoThrow : interestpolicy is %d",
-           interestPolicy.ordinal);
+  TcrMessageRegisterInterestRegex request(
+      new DataOutput(m_cacheImpl->createDataOutput()), m_fullPath, regex,
+      interestPolicy, isDurable, getAttributes().getCachingEnabled(),
+      receiveValues, m_tcrdm.get());
 
-  // TODO:
-  TcrMessageRegisterInterest request(
-      new DataOutput(m_cacheImpl->createDataOutput()), m_fullPath,
-      regex.c_str(), interestPolicy, isDurable,
-      getAttributes().getCachingEnabled(), receiveValues, m_tcrdm.get());
   std::recursive_mutex responseLock;
-  if (reply == nullptr) {
-    TcrMessageReply replyLocal(true, m_tcrdm.get());
-    auto values = std::make_shared<HashMapOfCacheable>();
-    auto exceptions = std::make_shared<HashMapOfException>();
-
+  std::unique_ptr<TcrChunkedResult> resultCollector;
+  TcrMessageReply replyLocal(true, m_tcrdm.get());
+  if (reply) {
+    if (auto chunkedInterestResponse = dynamic_cast<ChunkedInterestResponse*>(
+            reply->getChunkedResultHandler())) {
+      resultKeys = chunkedInterestResponse->getResultKeys();
+    } else if (auto chunkedGetAllResponse =
+                   dynamic_cast<ChunkedGetAllResponse*>(
+                       reply->getChunkedResultHandler())) {
+      resultKeys = chunkedGetAllResponse->getResultKeys();
+    }
+  } else {
     reply = &replyLocal;
-    if (interestPolicy.ordinal == InterestResultPolicy::KEYS_VALUES.ordinal) {
+    if (!resultKeys) {
+      resultKeys =
+          std::make_shared<std::vector<std::shared_ptr<CacheableKey>>>();
+    }
+    if (interestPolicy == InterestResultPolicy::KEYS_VALUES) {
+      auto values = std::make_shared<HashMapOfCacheable>();
+      auto exceptions = std::make_shared<HashMapOfException>();

Review comment:
       Same here - exceptions map looks like it's just a transient parameter to 
the `ChunkedGetAllResponse` ctor

##########
File path: cppcache/src/ThinClientRegion.cpp
##########
@@ -553,26 +509,11 @@ void ThinClientRegion::unregisterRegex(const std::string& 
regex) {
     throw IllegalArgumentException("Unregister regex string is empty");
   }
 
-  GfErrType err = unregisterRegexNoThrow(regex);
+  const auto err = unregisterRegexNoThrow(regex);
   throwExceptionIfError("Region::unregisterRegex", err);
 }
 
-void ThinClientRegion::unregisterAllKeys() {
-  auto pool = 
m_cacheImpl->getPoolManager().find(getAttributes().getPoolName());
-  if (pool != nullptr) {
-    if (!pool->getSubscriptionEnabled()) {
-      LOGERROR(
-          "Unregister all keys is supported only if "
-          "subscription-enabled attribute is true for pool " +
-          pool->getName());
-      throw UnsupportedOperationException(
-          "Unregister all keys is supported only if "
-          "pool subscription-enabled attribute is true.");
-    }
-  }
-  GfErrType err = unregisterRegexNoThrow(".*");
-  throwExceptionIfError("Region::unregisterAllKeys", err);
-}
+void ThinClientRegion::unregisterAllKeys() { unregisterRegex(kAllKeysRegex); }

Review comment:
       Very nice simplification.




-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to