[GitHub] [geode] jinmeiliao merged pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
jinmeiliao merged pull request #5249: URL: https://github.com/apache/geode/pull/5249 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: us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442594400
##
File path: cppcache/src/PdxType.cpp
##
@@ -568,16 +568,32 @@ bool PdxType::operator<(const PdxType& other) const {
return false;
}
-int32_t PdxType::hashcode() const {
+bool PdxType::operator==(const PdxType& other) const {
+ if (this->m_className != other.m_className){
+return false;
+ }
+
+ if(this->m_noJavaClass != other.m_noJavaClass){
+return false;
+ }
+
+ if(this->getTotalFields() != other.getTotalFields()){
+return false;
+ }
+
+ //TODO: Compare m_pdxFieldTypes & other.m_pdxFieldTypes;
+ return true;
+}
+
+size_t PdxType::hashcode() const {
std::hash strHash;
- auto result=strHash(this->m_className);
+ auto result = strHash(this->m_className);
- for (std::vector>::iterator it =
- m_pdxFieldTypes->begin();
+ for (auto it = m_pdxFieldTypes->begin();
Review comment:
Can we use a range `for` loop here.
##
File path: cppcache/src/PdxType.cpp
##
@@ -568,16 +568,32 @@ bool PdxType::operator<(const PdxType& other) const {
return false;
}
-int32_t PdxType::hashcode() const {
+bool PdxType::operator==(const PdxType& other) const {
+ if (this->m_className != other.m_className){
+return false;
+ }
+
+ if(this->m_noJavaClass != other.m_noJavaClass){
+return false;
+ }
+
+ if(this->getTotalFields() != other.getTotalFields()){
+return false;
+ }
+
+ //TODO: Compare m_pdxFieldTypes & other.m_pdxFieldTypes;
Review comment:
What's with the new TODO?
##
File path: cppcache/src/PdxType.hpp
##
@@ -205,10 +205,25 @@ class PdxType : public internal::DataSerializableInternal,
// This is for PdxType as key in std map.
bool operator<(const PdxType& other) const;
- int32_t hashcode() const;
+ bool operator==(const PdxType& other) const;
+
+ size_t hashcode() const;
};
} // namespace client
} // namespace geode
} // namespace apache
+namespace std {
+
+template <>
+struct hash {
+ typedef apache::geode::client::PdxType argument_type;
+ typedef size_t result_type;
+ result_type operator()(const argument_type& val) const {
+return val.hashcode();
Review comment:
Technically we can take the `hashcode` method off the `PdxType` and line
it here. The hash is only used for `std::hash` so let's not split it across the
two classes.
##
File path: cppcache/src/PdxType.hpp
##
@@ -205,10 +205,25 @@ class PdxType : public internal::DataSerializableInternal,
// This is for PdxType as key in std map.
bool operator<(const PdxType& other) const;
Review comment:
If we aren't going to use the std::map anymore perhaps we should remove
this `operator<`.
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:
us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442566625
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
And now I think we see why PdxTests::PdxType existed in the first place.
apache::geode::client::PdxType is hidden in the library. The following fixes
linker errors for me, in cppcache/src/PdxType.hpp:
```
- class PdxType
+ class APACHE_GEODE_EXPORT PdxType
```
But now we need to have the more philosophical question about whether or not
this is the right thing to do. If we _are_ going to export PdxType, the header
needs to move from cppcache/src to cppcache/include/geode. @pivotal-jbarrett
any opinion/suggestion?
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:
us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442566625
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
And now I think we see why PdxTests::PdxType existed in the first place.
apache::geode::client::PdxType is hidden in the library. The following fixes
linker errors for me, in cppcache/src/PdxType.hpp:
```
- class PdxType
+ class APACHE_GEODE_EXPORT PdxType
```
But now we need to have the more philosophical question about whether or not
this is the right thing to do. If we _are_ going to export _PdxType_, the
header needs to move from cppcache/src to cppcache/include/geode.
@pivotal-jbarrett any opinion/suggestion?
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:
us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442559744
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
> the following change worked for me on line 183 (the first spot I was
hitting a compiler gripe):
Nice, its simpler that the code I have just added. Now with the current
code, you should see the linker error I was not able to solve:
```
PdxJsonTypeTest.cpp:166: undefined reference to
`apache::geode::client::PdxType::PdxType(apache::geode::client::PdxTypeRegistry&,
std::__cxx11::basic_string, std::allocator
> const&, bool)'
...
and more linker errors from this point
```
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:
us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442558468
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
After commenting out the using statement for PdxTest::PdxType, I found
the declaration of std::hash wasn't correct in the code (trying to pass 1 param
to ctor). the following change worked for me on line 183 (the first spot I was
hitting a compiler gripe):
```-
EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+ EXPECT_EQ(std::hash{}(m_pdxType1),
std::hash{}(m_pdxType2));
```
I expect the rest are similar...
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442558585
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
The first linker error is the following:
```
PdxJsonTypeTest.cpp:166: undefined reference to
`apache::geode::client::PdxType::PdxType(apache::geode::client::PdxTypeRegistry&,
std::__cxx11::basic_string, std::allocator
> const&, bool)'
```
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:
us...@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442558468
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
After commenting out the using statement for PdxTest::PdxType, I found
the declaration of std::hash wasn't correct in the code (trying to pass 1 param
to ctor). the following change worked for me on line 183 (the first spot I was
hitting a compiler gripe):
- EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+ EXPECT_EQ(std::hash{}(m_pdxType1),
std::hash{}(m_pdxType2));
I expect the rest are similar...
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:
us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442558200
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
Thank to your comment I have realized the assertions are wrong. Thing is
that in the assertions I was using `m_pdxType1.hashcode()` instead of
`std::hash(m_pdxType1)`, that was a change I did right before adding
the commit with the tests to the PR and its causing the compilation to fail in
other place different than the linker error I was asking for. I have just added
the correct code of the assertions.
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:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442553693
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
Oh yeah forgot about that test object. It but me a few times too and
thought it should be renamed. I assumed you tried the fully qualified name on
the line 204 too?
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:
us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442548151
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
Thanks for the suggestion, but it did not solve the problem. I used the
fully qualified name in the lines you mention because there is other PdxType
class used in the namespace.
`using PdxTests::PdxType;`
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:
us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442548151
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
Thanks for the suggestion, but it did not solve the problem. I used the
fully qualified name in the lines you mention because there is other PdxType
class used in the namespace.
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:
us...@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442548151
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
Thanks for the suggestion, but it did not solve the problem. I used the
fully qualified name in the lines you mention because there is other PdxType
class imported used in the namespace.
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:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442543440
##
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast(region2->get("simpleObject")));
}
+TEST(PdxJsonTypeTest, testTwoObjectsWithSameFieldsHaveTheSameHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+.create()
+.region()
+.withName("region")
+.withType("REPLICATE")
+.execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_EQ(std::hash(m_pdxType1),std::hash(m_pdxType2));
+}
+
+TEST(PdxJsonTypeTest, testTwoObjectsWithDifferentFieldsHaveDifferentHash) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+ cluster.start();
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("REPLICATE")
+ .execute();
+
+ auto cache = cluster.createCache();
+
+ auto properties = std::make_shared();
+ CacheImpl cacheImpl(, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry();
+
+ apache::geode::client::PdxType m_pdxType1(pdxTypeRegistry,
gemfireJsonClassName,false);
+ apache::geode::client::PdxType m_pdxType2(pdxTypeRegistry,
gemfireJsonClassName,false);
+
+ m_pdxType1.addVariableLengthTypeField("bar0", "string",
PdxFieldTypes::STRING);
+ m_pdxType1.addVariableLengthTypeField("bar1", "string",
PdxFieldTypes::STRING);
+
+ m_pdxType2.addVariableLengthTypeField("bar2", "string",
PdxFieldTypes::STRING);
+ m_pdxType2.addVariableLengthTypeField("bar3", "string",
PdxFieldTypes::STRING);
+
+ EXPECT_NE(std::hash(m_pdxType1),std::hash(m_pdxType2));
Review comment:
I think you problem might be that `PdxType` wasn't imported into the
current namespace like many of the other types. Notice lines 195 and 196
reference it with the fully qualified name and this line does not. Add a
`using` statement at line 54 and then you should be good, and you can drop the
fully qualified names on 195, 196.
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:
us...@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #5139: GEODE-8149: New parameter and property introduced
pivotal-jbarrett commented on pull request #5139: URL: https://github.com/apache/geode/pull/5139#issuecomment-646339798 @jvarenina How about you close this PR until that is resolved then. 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: us...@infra.apache.org
[GitHub] [geode] aaronlindsey merged pull request #5236: GEODE-8241: Locator observes locator-wait-time
aaronlindsey merged pull request #5236: URL: https://github.com/apache/geode/pull/5236 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: us...@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
DonalEvans commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r442536212
##
File path:
geode-management/src/main/java/org/apache/geode/management/runtime/RestoreRedundancyResults.java
##
@@ -18,28 +18,28 @@
import java.util.List;
import java.util.Map;
+import org.apache.geode.annotations.Experimental;
+
/**
* A class to collect the results of restore redundancy operations for one or
more regions and
* determine the success of failure of the operation.
*/
+@Experimental
public interface RestoreRedundancyResults extends OperationResult {
/**
* {@link #SUCCESS} is defined as every included region having fully
satisfied redundancy.
* {@link #FAILURE} is defined as at least one region that is configured to
have redundant copies
* having fewer than its configured number of redundant copies.
- * {@link #ERROR} is for cases when the restore redundancy operation was
unable to begin or threw
- * an exception.
*/
enum Status {
SUCCESS,
-FAILURE,
-ERROR
Review comment:
You're correct, this was originally intended to be a way to capture the
case that an exception was thrown during the `doRestoreRedundancy()` method in
`RestoreRedundancyOperationImpl`, but in order to keep the implementation as
similar to that found in `RebalanceOperationImpl` as possible, this was not
implemented. It should be fine to remove it.
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:
us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5257: GEODE-8251: make sure Configuration can be deserialized post 1.12.
jinmeiliao commented on a change in pull request #5257:
URL: https://github.com/apache/geode/pull/5257#discussion_r442533132
##
File path:
geode-core/src/upgradeTest/java/org/apache/geode/internal/cache/rollingupgrade/RollingUpgrade2DUnitTestBase.java
##
@@ -1185,6 +1185,8 @@ Properties getLocatorProperties(String locatorsString,
boolean enableCC) {
props.setProperty(DistributionConfig.LOCATORS_NAME, locatorsString);
props.setProperty(DistributionConfig.LOG_LEVEL_NAME,
DUnitLauncher.logLevel);
props.setProperty(DistributionConfig.ENABLE_CLUSTER_CONFIGURATION_NAME,
enableCC + "");
+// do not start http service to avoid port conflict between upgrade tests
+props.setProperty(DistributionConfig.HTTP_SERVICE_PORT_NAME, "0");
Review comment:
here 0 means do not start.
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:
us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
jinmeiliao commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r442526334
##
File path:
geode-management/src/main/java/org/apache/geode/management/runtime/RestoreRedundancyResults.java
##
@@ -18,28 +18,28 @@
import java.util.List;
import java.util.Map;
+import org.apache.geode.annotations.Experimental;
+
/**
* A class to collect the results of restore redundancy operations for one or
more regions and
* determine the success of failure of the operation.
*/
+@Experimental
public interface RestoreRedundancyResults extends OperationResult {
/**
* {@link #SUCCESS} is defined as every included region having fully
satisfied redundancy.
* {@link #FAILURE} is defined as at least one region that is configured to
have redundant copies
* having fewer than its configured number of redundant copies.
- * {@link #ERROR} is for cases when the restore redundancy operation was
unable to begin or threw
- * an exception.
*/
enum Status {
SUCCESS,
-FAILURE,
-ERROR
Review comment:
@DonalEvans Please take a look at this change. We get rid of the ERROR
status here because we found it's never used. No one is able to set the status
to that state, and the getter of it would only return either SUCCESS or
FAILURE. Even on develop branch, this state seems unreachable.
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:
us...@infra.apache.org
[GitHub] [geode] prettyClouds opened a new pull request #5276: GEODE-8280: Return correct Redis AUTH errors
prettyClouds opened a new pull request #5276: URL: https://github.com/apache/geode/pull/5276 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, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
jinmeiliao commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r442502008
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/functions/RestoreRedundancyFunction.java
##
@@ -0,0 +1,105 @@
+/*
+ * 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.management.internal.functions;
+
+import static
org.apache.geode.management.runtime.RestoreRedundancyResults.Status.ERROR;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.apache.logging.log4j.Logger;
+
+import org.apache.geode.cache.control.RestoreRedundancyOperation;
+import org.apache.geode.cache.execute.FunctionContext;
+import
org.apache.geode.internal.cache.control.SerializableRestoreRedundancyResultsImpl;
+import org.apache.geode.internal.cache.execute.InternalFunction;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import
org.apache.geode.management.internal.operation.RestoreRedundancyResultsImpl;
+import org.apache.geode.management.operation.RestoreRedundancyRequest;
+
+
+public class RestoreRedundancyFunction implements InternalFunction {
+ private static final Logger logger = LogService.getLogger();
+
+ public static final String ID = RestoreRedundancyFunction.class.getName();
+
+
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ // this would return the RestoreRedundancyResults if successful,
+ // it will return an exception to the caller if status is failure or any
exception happens
+ public void execute(FunctionContext context) {
+Object[] arguments = context.getArguments();
+RestoreRedundancyRequest request = (RestoreRedundancyRequest) arguments[0];
+boolean isStatusCommand = (boolean) arguments[1];
+RestoreRedundancyOperation redundancyOperation =
+
context.getCache().getResourceManager().createRestoreRedundancyOperation();
+Set includeRegionsSet = null;
+if (request.getIncludeRegions() != null) {
+ includeRegionsSet = new HashSet<>(request.getIncludeRegions());
+}
+Set excludeRegionsSet = null;
+if (request.getExcludeRegions() != null) {
+ excludeRegionsSet = new HashSet<>(request.getExcludeRegions());
+}
+redundancyOperation.includeRegions(includeRegionsSet);
+redundancyOperation.excludeRegions(excludeRegionsSet);
+RestoreRedundancyResultsImpl results;
+
+try {
+ if (isStatusCommand) {
+results = (RestoreRedundancyResultsImpl)
redundancyOperation.redundancyStatus();
+ } else {
+
redundancyOperation.shouldReassignPrimaries(request.getReassignPrimaries());
+results = (RestoreRedundancyResultsImpl)
redundancyOperation.start().join();
+ }
+ if (results.getRegionOperationStatus().equals(ERROR)) {
+Exception e = new Exception(results.getRegionOperationMessage());
+throw e;
+ }
+ results.setSuccess(true);
+ results.setStatusMessage("Success"); // MLH change this
+} catch (Exception e) {
+ results =
+ new SerializableRestoreRedundancyResultsImpl();
+ results.setSuccess(false);
+ results.setStatusMessage(e.getMessage());
+}
Review comment:
Generally `SerializableRestoreRedundancyResultsImpl` is used as the data
object that's flowing from server to locator in order to support rolling
upgrade. It would be nice that the result of the function call are all in that
type.
##
File path:
geode-core/src/main/java/org/apache/geode/management/internal/functions/RestoreRedundancyFunction.java
##
@@ -0,0 +1,105 @@
+/*
+ * 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
[GitHub] [geode] jinmeiliao commented on pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
jinmeiliao commented on pull request #5249: URL: https://github.com/apache/geode/pull/5249#issuecomment-646304378 > I have a question about the packages and modules of the classes. e.g. `RestoreRedundancyResultsImpl` is in package `org.apache.geode.management.internal.operation`. And it is in `geode-management` module. Its subclass `SerializableRestoreRedundancyResultsImpl` is in a different package `org.apache.geode.internal.cache.control` and a different module `geode-core`. Why these two classes are in different packages and different modules? > Also some classes have been moved to a different package. e.g. `RegionRedundancyStatus` and `RestoreRedundancyResults`. Is there a reason for that? RestoreRedundancyResults is an interface and is external api, RestoreRedundancyResultsImpl is a direct implementation of it. These two are in the geode-management module because cms needs them to convey result data back to the client. "SerializableRestoreRedundancyResultsImpl", however, is needed to convey function call results from locator to servers, it needs to support rolling upgrade, so it needs to implement "FixedDataSerializableID" interface, that's why we have to have it extends RestoreRedundancyResultsImpl and implement the additional interfaces in geode-core. 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: us...@infra.apache.org
[GitHub] [geode] sabbeyPivotal opened a new pull request #5275: GEODE-8269: Improve test coverage
sabbeyPivotal opened a new pull request #5275: URL: https://github.com/apache/geode/pull/5275 Adding some additional tests that were missing. 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: us...@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #5273: GEODE-8240: solve rolling upgrade bug with new VersionOrdinal interface
Bill commented on a change in pull request #5273:
URL: https://github.com/apache/geode/pull/5273#discussion_r442452067
##
File path:
geode-serialization/src/main/java/org/apache/geode/internal/serialization/Version.java
##
@@ -635,48 +569,4 @@ public boolean isPre65() {
.collect(Collectors.toList());
}
- public boolean isCurrentVersion() {
-return this.ordinal == CURRENT.ordinal;
- }
Review comment:
this was used in exactly two places and is better accomplished via
`Version.CURRENT.isEqual(someVersionOrdinal)`
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:
us...@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #5273: GEODE-8240: solve rolling upgrade bug with new VersionOrdinal interface
Bill commented on a change in pull request #5273:
URL: https://github.com/apache/geode/pull/5273#discussion_r442451532
##
File path:
geode-serialization/src/main/java/org/apache/geode/internal/serialization/internal/DSFIDSerializerImpl.java
##
@@ -220,20 +220,6 @@ public void invokeToData(Object ds, DataOutput out) throws
IOException {
}
}
- /**
- * Get the Version of the peer or disk store that created this {@link
DataOutput}.
- * Returns
- * zero if the version is same as this member's.
- */
- public Version getVersionForDataStreamOrNull(DataOutput out) {
-// check if this is a versioned data output
-if (out instanceof VersionedDataStream) {
- return ((VersionedDataStream) out).getVersion();
-} else {
- return null;
-}
- }
Review comment:
dead code
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:
us...@infra.apache.org
[GitHub] [geode] nabarunnag merged pull request #5259: GEODE-8216: Changing the location of pausing the senders before the r…
nabarunnag merged pull request #5259: URL: https://github.com/apache/geode/pull/5259 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: us...@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #5271: Add Mass Test Run pipeline.
mhansonp commented on a change in pull request #5271: URL: https://github.com/apache/geode/pull/5271#discussion_r442430968 ## File path: ci/pipelines/shared/jinja.variables.yml ## @@ -67,6 +67,7 @@ java_test_versions: metadata: initial_version: 1.14.0-((semver-prerelease-token)).0 + mass_test_run_iterations: 200 Review comment: So you would use the SHA as normal, but you would run 100 tests and report against the last 200. That will give us a historical result set for the last two weeks. Or to just keep it simple, we can just do 100 and report on 100. 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: us...@infra.apache.org
[GitHub] [geode] rhoughton-pivot commented on a change in pull request #5271: Add Mass Test Run pipeline.
rhoughton-pivot commented on a change in pull request #5271: URL: https://github.com/apache/geode/pull/5271#discussion_r442429688 ## File path: ci/pipelines/shared/jinja.variables.yml ## @@ -67,6 +67,7 @@ java_test_versions: metadata: initial_version: 1.14.0-((semver-prerelease-token)).0 + mass_test_run_iterations: 200 Review comment: Why would I run 100 jobs of `abcdef` and then run a report containing 200 jobs mixed of `abcdef` and `123456` ? 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: us...@infra.apache.org
[GitHub] [geode] rhoughton-pivot commented on a change in pull request #5271: Add Mass Test Run pipeline.
rhoughton-pivot commented on a change in pull request #5271: URL: https://github.com/apache/geode/pull/5271#discussion_r442429688 ## File path: ci/pipelines/shared/jinja.variables.yml ## @@ -67,6 +67,7 @@ java_test_versions: metadata: initial_version: 1.14.0-((semver-prerelease-token)).0 + mass_test_run_iterations: 200 Review comment: Why would I run 100 jobs against SHA `abcdef` and then run a report containing 200 jobs mixed of `abcdef` and `123456` ? 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: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5249: GEODE-8272 Refactor Restore Redundancy Command
jinmeiliao commented on a change in pull request #5249:
URL: https://github.com/apache/geode/pull/5249#discussion_r442418701
##
File path:
geode-management/src/main/java/org/apache/geode/management/operation/RestoreRedundancyRequest.java
##
@@ -0,0 +1,123 @@
+/*
+ * 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.management.operation;
+
+import java.util.List;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.api.ClusterManagementOperation;
+import org.apache.geode.management.runtime.RestoreRedundancyResults;
+
+/**
+ * Defines a distributed system request to optimize bucket allocation across
members.
+ */
+@Experimental
+public class RestoreRedundancyRequest
+implements ClusterManagementOperation {
+
+ /**
+ * see {@link #getEndpoint()}
+ */
+ public static final String RESTORE_REDUNDANCY_REBALANCE_ENDPOINT =
+ "/operations/restoreRedundancy";
+ // null means all regions included
+ private List includeRegions;
+ // null means don't exclude any regions
+ private List excludeRegions;
+ private boolean reassignPrimaries = true;
+ private String operator;
+
+ /**
+ * by default, requests all partitioned regions to be rebalanced
+ */
+ public RestoreRedundancyRequest() {}
+
+ /**
+ * copy constructor
+ */
+ public RestoreRedundancyRequest(
+ RestoreRedundancyRequest other) {
+this.setExcludeRegions(other.getExcludeRegions());
+this.setIncludeRegions(other.getIncludeRegions());
+this.setReassignPrimaries(other.getReassignPrimaries());
+this.operator = other.getOperator();
+ }
+
+ /***
+ * Returns the list of regions to be rebalanced (or an empty list for
all-except-excluded)
+ */
+ public List getIncludeRegions() {
+return includeRegions;
+ }
+
+ /**
+ * requests rebalance of the specified region(s) only. When at least one
region is specified, this
+ * takes precedence over any excluded regions.
+ */
+ public void setIncludeRegions(List includeRegions) {
+this.includeRegions = includeRegions;
+ }
+
+ /***
+ * Returns the list of regions NOT to be rebalanced (iff {@link
#getIncludeRegions()} is empty)
+ */
+ public List getExcludeRegions() {
+return excludeRegions;
+ }
+
+ /**
+ * excludes specific regions from the rebalance, if {@link
#getIncludeRegions()} is empty,
+ * otherwise has no effect
+ * default: no regions are excluded
+ */
+ public void setExcludeRegions(List excludeRegions) {
+this.excludeRegions = excludeRegions;
+ }
+
+ public void setReassignPrimaries(boolean reassignPrimaries) {
+this.reassignPrimaries = reassignPrimaries;
+ }
+
+ public boolean getReassignPrimaries() {
+return reassignPrimaries;
+ }
+
+ @Override
+ @JsonIgnore
+ public String getEndpoint() {
+return RESTORE_REDUNDANCY_REBALANCE_ENDPOINT;
+ }
+
+ @Override
+ public String getOperator() {
+return operator;
+ }
+
+ public void setOperator(String operator) {
Review comment:
I believe you will need to set the operator in the controller, just like
what the rebalance did
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:
us...@infra.apache.org
[GitHub] [geode] aaronlindsey commented on pull request #5236: GEODE-8241: Locator observes locator-wait-time
aaronlindsey commented on pull request #5236: URL: https://github.com/apache/geode/pull/5236#issuecomment-646223094 I rebased on develop to pick up the fix for the failing `AcceptanceTestOpenJDK11` check. 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: us...@infra.apache.org
[GitHub] [geode] aaronlindsey commented on pull request #5236: GEODE-8241: Locator observes locator-wait-time
aaronlindsey commented on pull request #5236: URL: https://github.com/apache/geode/pull/5236#issuecomment-646219134 The failing `AcceptanceTestOpenJDK11` check is also failing on develop. 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: us...@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #5257: GEODE-8251: make sure Configuration can be deserialized post 1.12.
kirklund commented on a change in pull request #5257:
URL: https://github.com/apache/geode/pull/5257#discussion_r442402889
##
File path:
geode-management/src/main/java/org/apache/geode/management/configuration/AbstractConfiguration.java
##
@@ -47,7 +47,7 @@
@Experimental
public abstract class AbstractConfiguration
implements Identifiable, JsonSerializable {
-
+ private static final long serialVersionUID = -6612840641128145954L;
Review comment:
Optional: all JDK classes put serialVersionUID at the bottom of the
class. I usually do this to try and be consistent but obviously doesn't matter
much.
##
File path:
geode-core/src/upgradeTest/java/org/apache/geode/internal/cache/rollingupgrade/RollingUpgradeWithGfshDUnitTest.java
##
@@ -0,0 +1,160 @@
+/*
+ * 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.rollingupgrade;
+
+/*
+ * 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.
+ */
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.apache.geode.internal.UniquePortSupplier;
+import org.apache.geode.test.compiler.ClassBuilder;
+import org.apache.geode.test.junit.categories.BackwardCompatibilityTest;
+import org.apache.geode.test.junit.rules.gfsh.GfshExecution;
+import org.apache.geode.test.junit.rules.gfsh.GfshRule;
+import org.apache.geode.test.junit.rules.gfsh.GfshScript;
+import
org.apache.geode.test.junit.runners.CategoryWithParameterizedRunnerFactory;
+import org.apache.geode.test.version.TestVersion;
+import org.apache.geode.test.version.VersionManager;
+
+/**
+ * This test iterates through the versions of Geode and executes client
compatibility with
+ * the current version of Geode.
+ */
+@Category({BackwardCompatibilityTest.class})
+@RunWith(Parameterized.class)
+@Parameterized.UseParametersRunnerFactory(CategoryWithParameterizedRunnerFactory.class)
+public class RollingUpgradeWithGfshDUnitTest {
+ private final UniquePortSupplier portSupplier = new UniquePortSupplier();
+ private final String oldVersion;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static Collection data() {
+List result =
VersionManager.getInstance().getVersionsWithoutCurrent();
+result.removeIf(s -> TestVersion.compare(s, "1.10.0") < 0);
+return result;
+ }
+
+ @Rule
+ public GfshRule oldGfsh;
+
+ @Rule
+ public GfshRule currentGfsh = new GfshRule();
+
+ @Rule
+ public TemporaryFolder tempFolder = new TemporaryFolder();
+
+ public RollingUpgradeWithGfshDUnitTest(String version) {
+oldVersion = version;
+oldGfsh = new GfshRule(oldVersion);
+ }
+
+ @Test
+ public void testRollingUpgradeWithDeployment() throws Exception {
+int locatorPort = portSupplier.getAvailablePort();
+int locatorJmxPort = portSupplier.getAvailablePort();
+int locator2Port = portSupplier.getAvailablePort();
+int locator2JmxPort = portSupplier.getAvailablePort();
+int server1Port = portSupplier.getAvailablePort();
+int server2Port = portSupplier.getAvailablePort();
+
+GfshExecution startupExecution =
+GfshScript.of(startLocatorCommand("loc1", locatorPort, locatorJmxPort,
[GitHub] [geode] jdeppe-pivotal merged pull request #5262: GEODE-8269: Improve test coverage
jdeppe-pivotal merged pull request #5262: URL: https://github.com/apache/geode/pull/5262 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: us...@infra.apache.org
[GitHub] [geode] jdeppe-pivotal merged pull request #5272: GEODE-8276: fix KEYS command to handle non-ASCII keys
jdeppe-pivotal merged pull request #5272: URL: https://github.com/apache/geode/pull/5272 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: us...@infra.apache.org
[GitHub] [geode] bschuchardt merged pull request #5274: GEODE-8277: acceptance test certificates expired in Dockerized SNI acceptance tests
bschuchardt merged pull request #5274: URL: https://github.com/apache/geode/pull/5274 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: us...@infra.apache.org
[GitHub] [geode] jinmeiliao merged pull request #5231: GEODE-8237: Add note about 'alter region' & cluster conf service
jinmeiliao merged pull request #5231: URL: https://github.com/apache/geode/pull/5231 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: us...@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5268: GEODE-8250: Create new custom log config acceptance tests
jinmeiliao commented on a change in pull request #5268:
URL: https://github.com/apache/geode/pull/5268#discussion_r442387862
##
File path:
geode-assembly/src/acceptanceTest/java/org/apache/geode/logging/LocatorWithCustomLogConfigAcceptanceTest.java
##
@@ -0,0 +1,291 @@
+/*
+ * 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.logging;
+
+import static java.nio.file.Files.copy;
+import static
org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPorts;
+import static org.apache.geode.test.assertj.LogFileAssert.assertThat;
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.apache.geode.test.util.ResourceUtils.createFileFromResource;
+import static org.apache.geode.test.util.ResourceUtils.getResource;
+
+import java.nio.file.Path;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.rules.TestName;
+
+import org.apache.geode.test.junit.rules.RequiresGeodeHome;
+import org.apache.geode.test.junit.rules.gfsh.GfshRule;
+
+public class LocatorWithCustomLogConfigAcceptanceTest {
+
+ private static final String CONFIG_WITH_GEODE_PLUGINS_FILE_NAME =
+ "LocatorWithCustomLogConfigAcceptanceTestWithGeodePlugins.xml";
+ private static final String CONFIG_WITHOUT_GEODE_PLUGINS_FILE_NAME =
+ "LocatorWithCustomLogConfigAcceptanceTestWithoutGeodePlugins.xml";
+
+ private String locatorName;
+ private Path workingDir;
+ private int locatorPort;
+ private int httpPort;
+ private int rmiPort;
+ private Path configWithGeodePluginsFile;
+ private Path configWithoutGeodePluginsFile;
+ private Path locatorLogFile;
+ private Path pulseLogFile;
+ private Path customLogFile;
+
+ @Rule
+ public GfshRule gfshRule = new GfshRule();
+ @Rule
+ public RequiresGeodeHome requiresGeodeHome = new RequiresGeodeHome();
+ @Rule
+ public TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setUpLogConfigFiles() {
+configWithGeodePluginsFile = createFileFromResource(
+getResource(CONFIG_WITH_GEODE_PLUGINS_FILE_NAME),
temporaryFolder.getRoot(),
+CONFIG_WITH_GEODE_PLUGINS_FILE_NAME)
+.toPath();
+
+configWithoutGeodePluginsFile = createFileFromResource(
+getResource(CONFIG_WITHOUT_GEODE_PLUGINS_FILE_NAME),
temporaryFolder.getRoot(),
+CONFIG_WITHOUT_GEODE_PLUGINS_FILE_NAME)
+.toPath();
+ }
+
+ @Before
+ public void setUpOutputFiles() {
+locatorName = testName.getMethodName();
+
+workingDir = temporaryFolder.getRoot().toPath().toAbsolutePath();
+locatorLogFile = workingDir.resolve(locatorName + ".log");
+pulseLogFile = workingDir.resolve("pulse.log");
+customLogFile = workingDir.resolve("custom.log");
+ }
+
+ @Before
+ public void setUpRandomPorts() {
+int[] ports = getRandomAvailableTCPPorts(3);
+
+locatorPort = ports[0];
+httpPort = ports[1];
+rmiPort = ports[2];
+ }
+
+ @After
+ public void stopLocator() {
+gfshRule.execute("stop locator --dir=" + workingDir);
Review comment:
GfshRule itself will try to stop all the processes it starts using the
rule if you allow it to host the working dir of all the servers/locators it
started. Using the gfshRule.getWorkingDir will get the temp folder that gfsh
uses and all the servers/locators' working dir will be under there. That way,
when tests finish, GfshRule will kill all the members automatically.
##
File path:
geode-assembly/src/acceptanceTest/java/org/apache/geode/logging/LocatorWithCustomLogConfigAcceptanceTest.java
##
@@ -0,0 +1,291 @@
+/*
+ * 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
+
[GitHub] [geode-native] pdxcodemonkey commented on pull request #619: GEODE-8212: Reduce connections to server to get type id
pdxcodemonkey commented on pull request #619: URL: https://github.com/apache/geode-native/pull/619#issuecomment-646187772 Sure, I'll have a look 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: us...@infra.apache.org
[GitHub] [geode] bschuchardt opened a new pull request #5274: GEODE-8277: acceptance test certificates expired in Dockerized SNI acceptance tests
bschuchardt opened a new pull request #5274: URL: https://github.com/apache/geode/pull/5274 The old keystores have expired. I've generated 100 year keystores and added a program to recreate them if necessary. I've inspected the new jks files to verify that they expire in 2120 and have run the SNI tests to ensure that they still work. 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, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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: us...@infra.apache.org
[GitHub] [geode] sabbeyPivotal commented on pull request #5262: GEODE-8269: Improve test coverage
sabbeyPivotal commented on pull request #5262: URL: https://github.com/apache/geode/pull/5262#issuecomment-646107055 AcceptanceTestOpenJDK11 failure is not a result of these code changes. 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: us...@infra.apache.org
[GitHub] [geode] Bill opened a new pull request #5273: GEODE-8240: solve rolling upgrade bug with new VersionOrdinal interface
Bill opened a new pull request #5273:
URL: https://github.com/apache/geode/pull/5273
[GEODE-8240](https://issues.apache.org/jira/browse/GEODE-8240)
This is an alternative solution to the one explored in
https://github.com/apache/geode/pull/5269. This PR introduces a new interface
`VersionOrdinal` to model not just versions we know about (as `Version` does)
but also future versions. Formerly Geode modeled these as `short` but that
presented difficulties in the many places where we need to compare versions. By
unifying known and unknown versions under one interface, capable of providing
generic comparison (`compareTo()`, `equals()`, and `hashCode()`) my hope is
that we can eliminate this egregious loss of information (in that other PR's
`Version` implementation):
```java
@Override
public Version getVersion() {
public Version getVersion() {
return versionObj;
try {
return Version.fromOrdinal(versionOrdinal);
} catch (final UnsupportedSerializationVersionException e) {
return Version.CURRENT;
}
```
### 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)?
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:
us...@infra.apache.org
[GitHub] [geode] aaronlindsey commented on pull request #5236: GEODE-8241: Locator observes locator-wait-time
aaronlindsey commented on pull request #5236: URL: https://github.com/apache/geode/pull/5236#issuecomment-646043919 @Bill @bschuchardt with the previous changes I noticed a failure after 400-something runs. The chosen `locatorWaitTime` was not quite long enough. So I greatly increased the `locatorWaitTime` in the latest commit and then I was able to run the test 1000x successfully. I'm pretty confident that after the latest commit the chance of flakiness is negligible. 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: us...@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #5269: GEODE-8240: View has old locator version number after rolling upgrade
Bill commented on a change in pull request #5269:
URL: https://github.com/apache/geode/pull/5269#discussion_r442241602
##
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMemberData.java
##
@@ -220,12 +220,16 @@ public InetAddress getInetAddress() {
@Override
public short getVersionOrdinal() {
-return this.versionObj.ordinal();
+return versionOrdinal;
}
@Override
public Version getVersion() {
-return versionObj;
+try {
+ return Version.fromOrdinal(versionOrdinal);
+} catch (final UnsupportedSerializationVersionException e) {
Review comment:
**This right here** is a big fat problem. We are silently throwing away
information when the version ordinal represents an unknown version. One
approach is to let the exception escape (as a checked exception.) But that
entails fixing the 53 callers of this method. An alternative would be to
"tunnel" the checked exception through an unchecked one.
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:
us...@infra.apache.org
[GitHub] [geode-native] albertogpz closed pull request #611: GEODE-8213: lock-free SerializationRegistry
albertogpz closed pull request #611: URL: https://github.com/apache/geode-native/pull/611 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: us...@infra.apache.org
[GitHub] [geode-native] albertogpz commented on pull request #611: GEODE-8213: lock-free SerializationRegistry
albertogpz commented on pull request #611: URL: https://github.com/apache/geode-native/pull/611#issuecomment-645861561 > I think we should close this PR and revisit if after the fixes in #619 to be merged. After that I think It might just be safe to start by replacing the spinlock with just a plain old mutex to reduce the CPU load. It may not bench as good but solves the issue of CPU load under contention. I agree 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: us...@infra.apache.org
[GitHub] [geode] dschneider-pivotal merged pull request #5260: GEODE-8264: add serialization tests for RedisData classes
dschneider-pivotal merged pull request #5260: URL: https://github.com/apache/geode/pull/5260 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: us...@infra.apache.org
[GitHub] [geode] jvarenina commented on pull request #5139: GEODE-8149: New parameter and property introduced
jvarenina commented on pull request #5139: URL: https://github.com/apache/geode/pull/5139#issuecomment-645803468 Hi, Yes, property is introduced for future works. The reason I didn't update this PR is because we faced some issues when developing this feature which could affect this PR greatly when resolved. We have problem with retrieving certificates from RMI connections(e.g. jmx connections) and I will send more info related to this issue in dev list. We have also put on hold all other task related to this feature until we find the solution for RMI Connections. @davebarnes97 thanks again for comments I will try to implement them as soon as possible. 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: us...@infra.apache.org
