You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/06/09 16:42:07 UTC
[GitHub] [geode-native] albertogpz opened a new pull request #615: GEODE-8231: remove bucket server location from metadata when server down
albertogpz opened a new pull request #615:
URL: https://github.com/apache/geode-native/pull/615
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r443683084
##########
File path: cppcache/integration/test/CMakeLists.txt
##########
@@ -27,6 +27,7 @@ add_executable(cpp-integration-test
ExampleTest.cpp
ExpirationTest.cpp
FunctionExecutionTest.cpp
+ PartitionRegionOpsWithRedundancyAndServerGoesDown.cpp
Review comment:
I tend to always work in an IDE that supports project view (Visual Studio, Xcode), wherein I already know I'm working with a test project. Hence the Test suffix is a bit redundant. Also, we already have 4 tests in the new framework that don't use the Test suffix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell merged pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
mmartell merged pull request #615:
URL: https://github.com/apache/geode-native/pull/615
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r443683323
##########
File path: cppcache/integration/test/PartitionRegionOpsTest.cpp
##########
@@ -0,0 +1,169 @@
+/*
+ * 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 <chrono>
+#include <future>
+#include <iostream>
+#include <random>
+#include <thread>
+
+#include <gtest/gtest.h>
+
+#include <geode/Cache.hpp>
+#include <geode/PoolManager.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+#include "CacheRegionHelper.hpp"
+#include "framework/Cluster.h"
+#include "framework/Framework.h"
+#include "framework/Gfsh.h"
+
+namespace {
+
+using apache::geode::client::Cache;
+using apache::geode::client::Cacheable;
+using apache::geode::client::CacheableKey;
+using apache::geode::client::CacheableString;
+using apache::geode::client::HashMapOfCacheable;
+using apache::geode::client::Pool;
+using apache::geode::client::Region;
+using apache::geode::client::RegionShortcut;
+
+using std::chrono::minutes;
+
+Cache createCache() {
+ using apache::geode::client::CacheFactory;
+
+ auto cache = CacheFactory()
+ .set("log-level", "debug")
+ .set("statistic-sampling-enabled", "false")
+ .create();
+
+ return cache;
+}
+
+std::shared_ptr<Pool> createPool(Cluster& cluster, Cache& cache) {
+ auto poolFactory = cache.getPoolManager().createFactory();
+ cluster.applyLocators(poolFactory);
+ poolFactory.setPRSingleHopEnabled(true);
+ return poolFactory.create("default");
+}
+
+std::shared_ptr<Region> setupRegion(Cache& cache,
+ const std::shared_ptr<Pool>& pool) {
+ auto region = cache.createRegionFactory(RegionShortcut::PROXY)
+ .setPoolName(pool->getName())
+ .create("region");
+
+ return region;
+}
+
+void putEntries(std::shared_ptr<Region> region, int numEntries,
Review comment:
I would call this method `putAndGetEntries`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] albertogpz commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r443731935
##########
File path: cppcache/integration/test/PartitionRegionOpsTest.cpp
##########
@@ -0,0 +1,169 @@
+/*
+ * 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 <chrono>
+#include <future>
+#include <iostream>
+#include <random>
+#include <thread>
+
+#include <gtest/gtest.h>
+
+#include <geode/Cache.hpp>
+#include <geode/PoolManager.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+#include "CacheRegionHelper.hpp"
+#include "framework/Cluster.h"
+#include "framework/Framework.h"
+#include "framework/Gfsh.h"
+
+namespace {
+
+using apache::geode::client::Cache;
+using apache::geode::client::Cacheable;
+using apache::geode::client::CacheableKey;
+using apache::geode::client::CacheableString;
+using apache::geode::client::HashMapOfCacheable;
+using apache::geode::client::Pool;
+using apache::geode::client::Region;
+using apache::geode::client::RegionShortcut;
+
+using std::chrono::minutes;
+
+Cache createCache() {
+ using apache::geode::client::CacheFactory;
+
+ auto cache = CacheFactory()
+ .set("log-level", "debug")
+ .set("statistic-sampling-enabled", "false")
+ .create();
+
+ return cache;
+}
+
+std::shared_ptr<Pool> createPool(Cluster& cluster, Cache& cache) {
+ auto poolFactory = cache.getPoolManager().createFactory();
+ cluster.applyLocators(poolFactory);
+ poolFactory.setPRSingleHopEnabled(true);
+ return poolFactory.create("default");
+}
+
+std::shared_ptr<Region> setupRegion(Cache& cache,
+ const std::shared_ptr<Pool>& pool) {
+ auto region = cache.createRegionFactory(RegionShortcut::PROXY)
+ .setPoolName(pool->getName())
+ .create("region");
+
+ return region;
+}
+
+void putEntries(std::shared_ptr<Region> region, int numEntries,
Review comment:
I prefer to remove the gets from the method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] albertogpz commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r443732165
##########
File path: cppcache/integration/test/PartitionRegionOpsWithRedundancyAndServerGoesDown.cpp
##########
@@ -0,0 +1,171 @@
+/*
+ * 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 <chrono>
+#include <future>
+#include <iostream>
+#include <random>
+#include <thread>
+
+#include <gtest/gtest.h>
+
+#include <geode/Cache.hpp>
+#include <geode/PoolManager.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+#include "CacheRegionHelper.hpp"
+#include "framework/Cluster.h"
+#include "framework/Framework.h"
+#include "framework/Gfsh.h"
+
+namespace {
+
+using apache::geode::client::Cache;
+using apache::geode::client::Cacheable;
+using apache::geode::client::CacheableKey;
+using apache::geode::client::CacheableString;
+using apache::geode::client::HashMapOfCacheable;
+using apache::geode::client::Pool;
+using apache::geode::client::Region;
+using apache::geode::client::RegionShortcut;
+
+using std::chrono::minutes;
+
+Cache createCache() {
+ using apache::geode::client::CacheFactory;
+
+ auto cache = CacheFactory()
+ .set("log-level", "debug")
+ .set("statistic-sampling-enabled", "false")
+ .create();
+
+ return cache;
+}
+
+std::shared_ptr<Pool> createPool(Cluster& cluster, Cache& cache) {
+ auto poolFactory = cache.getPoolManager().createFactory();
+ cluster.applyLocators(poolFactory);
+ poolFactory.setPRSingleHopEnabled(true);
+ return poolFactory.create("default");
+}
+
+std::shared_ptr<Region> setupRegion(Cache& cache,
+ const std::shared_ptr<Pool>& pool) {
+ auto region = cache.createRegionFactory(RegionShortcut::PROXY)
+ .setPoolName(pool->getName())
+ .create("region");
+
+ return region;
+}
+
+void putEntries(std::shared_ptr<Region> region, int numEntries,
+ int offsetForValue) {
+ for (int i = 0; i < numEntries; i++) {
+ auto key = CacheableKey::create(i);
+ region->put(key, Cacheable::create(std::to_string(i + offsetForValue)));
+ auto value = region->get(key);
Review comment:
Thanks. I'll remove the gets from the method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r438502781
##########
File path: cppcache/integration/test/CMakeLists.txt
##########
@@ -27,6 +27,7 @@ add_executable(cpp-integration-test
ExampleTest.cpp
ExpirationTest.cpp
FunctionExecutionTest.cpp
+ PartitionRegionOpsWithRedundancyAndServerGoesDown.cpp
Review comment:
I think it general we've been having test files end in `Test` just to make it super obvious when browsing files it is a test. Also the file reads more like a test case rather than an identifier for a set of tests. I think the test file being called `PartitionRegionOpsTest.cpp` would work since you use `getPartitionedRegionWithRedundancyServerGoesDown` and putPartitionedRegionWithRedundancyServerGoesDown` as your test names. We can see if others have thoughts about naming
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] albertogpz commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r440889688
##########
File path: cppcache/src/ClientMetadata.cpp
##########
@@ -114,14 +114,24 @@ const std::string& ClientMetadata::getColocatedWith() {
return m_colocatedWith;
}
+void ClientMetadata::removeBucketServerLocation(
+ const std::shared_ptr<BucketServerLocation>& serverLocation) {
+ for (auto&& locations : m_bucketServerLocationsList) {
+ for (unsigned int i = 0; i < locations.size(); i++) {
+ if (locations[i]->getEpString() == (serverLocation->getEpString())) {
+ locations.erase(locations.begin() + i);
Review comment:
Yes it will but given that when an element is erased we right after exit the loop with the break sentence then it does not matter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] albertogpz commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r438587313
##########
File path: cppcache/integration/test/CMakeLists.txt
##########
@@ -27,6 +27,7 @@ add_executable(cpp-integration-test
ExampleTest.cpp
ExpirationTest.cpp
FunctionExecutionTest.cpp
+ PartitionRegionOpsWithRedundancyAndServerGoesDown.cpp
Review comment:
I agree with you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] vjr commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
vjr commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r441266912
##########
File path: cppcache/src/ClientMetadata.cpp
##########
@@ -114,14 +114,24 @@ const std::string& ClientMetadata::getColocatedWith() {
return m_colocatedWith;
}
+void ClientMetadata::removeBucketServerLocation(
+ const std::shared_ptr<BucketServerLocation>& serverLocation) {
+ for (auto&& locations : m_bucketServerLocationsList) {
+ for (unsigned int i = 0; i < locations.size(); i++) {
+ if (locations[i]->getEpString() == (serverLocation->getEpString())) {
+ locations.erase(locations.begin() + i);
Review comment:
Oh yes, there is a `break` statement, I am blind!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r443698746
##########
File path: cppcache/integration/test/PartitionRegionOpsWithRedundancyAndServerGoesDown.cpp
##########
@@ -0,0 +1,171 @@
+/*
+ * 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 <chrono>
+#include <future>
+#include <iostream>
+#include <random>
+#include <thread>
+
+#include <gtest/gtest.h>
+
+#include <geode/Cache.hpp>
+#include <geode/PoolManager.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+#include "CacheRegionHelper.hpp"
+#include "framework/Cluster.h"
+#include "framework/Framework.h"
+#include "framework/Gfsh.h"
+
+namespace {
+
+using apache::geode::client::Cache;
+using apache::geode::client::Cacheable;
+using apache::geode::client::CacheableKey;
+using apache::geode::client::CacheableString;
+using apache::geode::client::HashMapOfCacheable;
+using apache::geode::client::Pool;
+using apache::geode::client::Region;
+using apache::geode::client::RegionShortcut;
+
+using std::chrono::minutes;
+
+Cache createCache() {
+ using apache::geode::client::CacheFactory;
+
+ auto cache = CacheFactory()
+ .set("log-level", "debug")
+ .set("statistic-sampling-enabled", "false")
+ .create();
+
+ return cache;
+}
+
+std::shared_ptr<Pool> createPool(Cluster& cluster, Cache& cache) {
+ auto poolFactory = cache.getPoolManager().createFactory();
+ cluster.applyLocators(poolFactory);
+ poolFactory.setPRSingleHopEnabled(true);
+ return poolFactory.create("default");
+}
+
+std::shared_ptr<Region> setupRegion(Cache& cache,
+ const std::shared_ptr<Pool>& pool) {
+ auto region = cache.createRegionFactory(RegionShortcut::PROXY)
+ .setPoolName(pool->getName())
+ .create("region");
+
+ return region;
+}
+
+void putEntries(std::shared_ptr<Region> region, int numEntries,
+ int offsetForValue) {
+ for (int i = 0; i < numEntries; i++) {
+ auto key = CacheableKey::create(i);
+ region->put(key, Cacheable::create(std::to_string(i + offsetForValue)));
+ auto value = region->get(key);
Review comment:
Nice test!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r443694624
##########
File path: cppcache/integration/test/PartitionRegionOpsWithRedundancyAndServerGoesDown.cpp
##########
@@ -0,0 +1,171 @@
+/*
+ * 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 <chrono>
+#include <future>
+#include <iostream>
+#include <random>
+#include <thread>
+
+#include <gtest/gtest.h>
+
+#include <geode/Cache.hpp>
+#include <geode/PoolManager.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+#include "CacheRegionHelper.hpp"
+#include "framework/Cluster.h"
+#include "framework/Framework.h"
+#include "framework/Gfsh.h"
+
+namespace {
+
+using apache::geode::client::Cache;
+using apache::geode::client::Cacheable;
+using apache::geode::client::CacheableKey;
+using apache::geode::client::CacheableString;
+using apache::geode::client::HashMapOfCacheable;
+using apache::geode::client::Pool;
+using apache::geode::client::Region;
+using apache::geode::client::RegionShortcut;
+
+using std::chrono::minutes;
+
+Cache createCache() {
+ using apache::geode::client::CacheFactory;
+
+ auto cache = CacheFactory()
+ .set("log-level", "debug")
+ .set("statistic-sampling-enabled", "false")
+ .create();
+
+ return cache;
+}
+
+std::shared_ptr<Pool> createPool(Cluster& cluster, Cache& cache) {
+ auto poolFactory = cache.getPoolManager().createFactory();
+ cluster.applyLocators(poolFactory);
+ poolFactory.setPRSingleHopEnabled(true);
+ return poolFactory.create("default");
+}
+
+std::shared_ptr<Region> setupRegion(Cache& cache,
+ const std::shared_ptr<Pool>& pool) {
+ auto region = cache.createRegionFactory(RegionShortcut::PROXY)
+ .setPoolName(pool->getName())
+ .create("region");
+
+ return region;
+}
+
+void putEntries(std::shared_ptr<Region> region, int numEntries,
+ int offsetForValue) {
+ for (int i = 0; i < numEntries; i++) {
+ auto key = CacheableKey::create(i);
+ region->put(key, Cacheable::create(std::to_string(i + offsetForValue)));
+ auto value = region->get(key);
Review comment:
I'd rename this function to putAndGetEntries. Or perhaps remove the gets, since you already have a getEntries function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] vjr commented on a change in pull request #615: GEODE-8231: remove bucket server location from metadata when server down
Posted by GitBox <gi...@apache.org>.
vjr commented on a change in pull request #615:
URL: https://github.com/apache/geode-native/pull/615#discussion_r440843882
##########
File path: cppcache/src/ClientMetadata.cpp
##########
@@ -114,14 +114,24 @@ const std::string& ClientMetadata::getColocatedWith() {
return m_colocatedWith;
}
+void ClientMetadata::removeBucketServerLocation(
+ const std::shared_ptr<BucketServerLocation>& serverLocation) {
+ for (auto&& locations : m_bucketServerLocationsList) {
+ for (unsigned int i = 0; i < locations.size(); i++) {
+ if (locations[i]->getEpString() == (serverLocation->getEpString())) {
+ locations.erase(locations.begin() + i);
Review comment:
Won't erasing an item from `locations` make it invalid in the `for` loop's `locations.size()` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org