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/16 14:22:09 UTC
[GitHub] [geode-native] alb3rtobr opened a new pull request #619: GEODE-8212: Reduce connections to server to get type id
alb3rtobr opened a new pull request #619:
URL: https://github.com/apache/geode-native/pull/619
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441715962
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector<std::shared_ptr<PdxFieldType>>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+ auto pdxPtr = *it;
+ result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
> Do we need to xor in the classname each time through the loop? It seems like just using the field names in here would be more appropriate
I added it because I thought in the case of two different classes having the same fields (names & types).
> One thing that is essential we get right here is that ordering of fields should have no bearing on the generated hash. so { "foo": 7, "bar" 10 } is the same type as { "bar": 12, "foo": 11 }, e.g. Otherwise we end up with a "type explosion" of zillions of copies of the same thing each getting their own type ID. If we're lucky, the server side will sort the field names when generating the type ID on that side, and they'll all get the same ID, but then we've still made a ton of unnecessary trips to the server.
In the tests I did, I saw that the order of the fields does not change the result of the hash.
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<PdxType>(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:
users@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
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #619:
URL: https://github.com/apache/geode-native/pull/619#issuecomment-646633422
As an experiment, I made the following change this morning:
```
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<Properties>();
// CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
PdxTypeRegistry pdxTypeRegistry(nullptr);
(...)
```
Since the new tests don't hit any code that dereferences the type registry, this worked, and the test(s) passed. This would allow the hash tests to be moved to the _unit_ test suite, where you have access to internal headers, and we wouldn't have to expose `apache::geode::client::PdxType` from the library. @pivotal-jbarrett , thoughts?
----------------------------------------------------------------
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] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<PdxType>(m_pdxType1),std::hash<PdxType>(m_pdxType2));
+ EXPECT_EQ(std::hash<PdxType>{}(m_pdxType1), std::hash<PdxType>{}(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:
users@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
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<char, std::char_traits<char>, std::allocator<char> > 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:
users@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
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442947626
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -537,25 +537,6 @@ void PdxType::generatePositionMap() {
}
}
-bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
Review comment:
Yay for deleted 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:
users@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
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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:
users@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
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r443618044
##########
File path: cppcache/benchmark/CMakeLists.txt
##########
@@ -19,12 +19,15 @@ add_executable(cpp-benchmark
GeodeHashBM.cpp
GeodeLoggingBM.cpp
NoopBM.cpp
- )
+ PdxTypeBM.cpp)
+
+#add_dependencies(integration-framework)
target_link_libraries(cpp-benchmark
PUBLIC
apache-geode-static
benchmark::benchmark
+ integration-framework
Review comment:
done!
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r443627658
##########
File path: cppcache/integration/benchmark/PdxTypeBM.cpp
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <benchmark/benchmark.h>
+
Review comment:
Sorry but Im not able to see any error, could you point me to the issue? Is your comment about the first include?
----------------------------------------------------------------
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 pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #619:
URL: https://github.com/apache/geode-native/pull/619#issuecomment-648001141
@pivotal-jbarrett & @pdxcodemonkey thanks for your help with this PR!
----------------------------------------------------------------
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 pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #619:
URL: https://github.com/apache/geode-native/pull/619#issuecomment-646185173
Could you help me with the tests? I have not been able to compile them, there is a linker error saying there is an undefined reference to `PdxType` but I dont know how to change the cmake config to fix it. Thanks in advance!
----------------------------------------------------------------
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] pdxcodemonkey commented on pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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:
users@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
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441751119
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector<std::shared_ptr<PdxFieldType>>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+ auto pdxPtr = *it;
+ result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
It would be good to have a unit test that shows the hashcode doesn't change if you iterate the fields in a different order...
----------------------------------------------------------------
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] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442946064
##########
File path: cppcache/test/CMakeLists.txt
##########
@@ -55,7 +56,7 @@ add_executable(apache-geode_unittests
util/synchronized_setTest.cpp
util/TestableRecursiveMutex.hpp
util/chrono/durationTest.cpp
-)
+ PdxTypeTest.cpp)
Review comment:
We should check and see if `tests/cpp/testobject/PdxType.*` are in use anywhere. We may get lucky and find that they can be deleted.
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441801421
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector<std::shared_ptr<PdxFieldType>>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+ auto pdxPtr = *it;
+ result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
> It would be good to have a unit test that shows the hashcode doesn't change if you iterate the fields in a different order...
sure, Im working on that :)
----------------------------------------------------------------
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] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r443622077
##########
File path: cppcache/integration/benchmark/PdxTypeBM.cpp
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <benchmark/benchmark.h>
+
Review comment:
This path seems incorrect.
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<char, std::char_traits<char>, std::allocator<char> > 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:
users@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
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442936246
##########
File path: cppcache/src/PdxType.hpp
##########
@@ -202,11 +202,38 @@ class PdxType : public internal::DataSerializableInternal,
bool Equals(std::shared_ptr<PdxType> otherObj);
- // This is for PdxType as key in std map.
- bool operator<(const PdxType& other) const;
+ bool operator==(const PdxType& other) const;
+
+ size_t hashcode() const;
Review comment:
Now that `std::hash` has the implementation is it necessary to have this on the class declaration? I don't see it in defined in the cpp file anymore.
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -556,12 +556,23 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
return true;
}
-bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+bool PdxType::operator==(const PdxType& other) const {
Review comment:
How does this differ from the `PdxType::Equals` method? Is there a purpose for the `PdxType::Equals` anymore?
##########
File path: cppcache/test/CMakeLists.txt
##########
@@ -55,7 +56,7 @@ add_executable(apache-geode_unittests
util/synchronized_setTest.cpp
util/TestableRecursiveMutex.hpp
util/chrono/durationTest.cpp
-)
+ PdxTypeTest.cpp)
Review comment:
Looks like we are adding this file twice.
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441807945
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
Review comment:
> Just a thought here. Is it necessary to hash like Java client does in this case?
No, we just need a hash function to allow identify `PdxType` objects that will be assigned the same id by the server. We dont need to know the hash generated by the server.
----------------------------------------------------------------
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] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441799345
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
Review comment:
Just a thought here. Is it necessary to hash like Java client does in this case? If the `PdxType::hashcode()` is expected to hash similarly to that of Java version then changes are needed here. If not then I wouldn't try to implement the `CacheableKey::hashcode` method here and do something more compatible with C++, like `std::hash` which produces a `size_t`. In fact I would just create a specialization of `std::hash<PdxType>`.
##########
File path: cppcache/src/PdxTypeRegistry.hpp
##########
@@ -53,7 +53,20 @@ typedef std::unordered_map<std::shared_ptr<PdxSerializable>,
dereference_hash<std::shared_ptr<CacheableKey>>,
dereference_equal_to<std::shared_ptr<CacheableKey>>>
PreservedHashMap;
-typedef std::map<std::shared_ptr<PdxType>, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr<PdxType> const& pdx) const {
+ return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr<PdxType> const& first, std::shared_ptr<PdxType> const& second) const{
+ return first->hashcode() == second->hashcode();
Review comment:
Equality of hashcode does not guarantee equality of the object. You should just implement `PdxType::operator==`.
##########
File path: cppcache/src/PdxTypeRegistry.hpp
##########
@@ -53,7 +53,20 @@ typedef std::unordered_map<std::shared_ptr<PdxSerializable>,
dereference_hash<std::shared_ptr<CacheableKey>>,
dereference_equal_to<std::shared_ptr<CacheableKey>>>
PreservedHashMap;
-typedef std::map<std::shared_ptr<PdxType>, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr<PdxType> const& pdx) const {
+ return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr<PdxType> const& first, std::shared_ptr<PdxType> const& second) const{
+ return first->hashcode() == second->hashcode();
+ }
+};
+
+typedef std::unordered_map<std::shared_ptr<PdxType>, int32_t, PdxTypeHashCode, PdxTypeEqualCmp>
Review comment:
If you implement `std::hash<PdxType>` and `PdxType::operator==` then you can use the `dereference_hash<std::shared_ptr<PdxType>>` and `dereference_equal_to<std::shared_ptr< PdxType >>>` here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441056111
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector<std::shared_ptr<PdxFieldType>>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+ auto pdxPtr = *it;
+ result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
One thing that is essential we get right here is that ordering of fields should have no bearing on the generated hash. so { "foo": 7, "bar" 10 } is the same type as { "bar": 12, "foo": 11 }, e.g. Otherwise we end up with a "type explosion" of zillions of copies of the same thing each getting their own type ID. If we're lucky, the server side will sort the field names when generating the type ID on that side, and they'll all get the same ID, but then we've still made a ton of unnecessary trips to the server.
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector<std::shared_ptr<PdxFieldType>>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+ auto pdxPtr = *it;
+ result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
Do we need to xor in the classname each time through the loop? It seems like just using the field names in here would be more appropriate
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442945650
##########
File path: cppcache/test/CMakeLists.txt
##########
@@ -55,7 +56,7 @@ add_executable(apache-geode_unittests
util/synchronized_setTest.cpp
util/TestableRecursiveMutex.hpp
util/chrono/durationTest.cpp
-)
+ PdxTypeTest.cpp)
Review comment:
Right, CLion added it for me and it added it manually
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441807945
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
Review comment:
> Just a thought here. Is it necessary to hash like Java client does in this case?
No, we just need a hash function to allow identify `PdxType` object that will be assigned the same id by the server. We dont need to know the hash generated by the server.
----------------------------------------------------------------
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] pivotal-jbarrett commented on pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #619:
URL: https://github.com/apache/geode-native/pull/619#issuecomment-646931189
@alb3rtobr If you are struggling with formatting make sure your `clang-format` is v6. Unfortunately the versions of `clang-format` disagree on some things.
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442720327
##########
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<apache::geode::client::PdxType> {
+ 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:
done!
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441823764
##########
File path: cppcache/src/PdxTypeRegistry.hpp
##########
@@ -53,7 +53,20 @@ typedef std::unordered_map<std::shared_ptr<PdxSerializable>,
dereference_hash<std::shared_ptr<CacheableKey>>,
dereference_equal_to<std::shared_ptr<CacheableKey>>>
PreservedHashMap;
-typedef std::map<std::shared_ptr<PdxType>, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr<PdxType> const& pdx) const {
+ return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr<PdxType> const& first, std::shared_ptr<PdxType> const& second) const{
+ return first->hashcode() == second->hashcode();
Review comment:
Agree, but I dont want to check if the objects are equals, I just want to check if two `PdxType` objects have the same hash.
Two `PdxType` objects should be equals if they have the same classname and their fields have the same types, names and values.
And two keys of the `pdxTypeToTypeId` map will be equals if they have the same hash, which means they have the same classname, and their fields have the same types and names (values are not checked here).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@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
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442686964
##########
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:
I wanted to separate in a different commit the check of the fields, as I was not sure how to do it. I was exploring the possibility to change `m_pdxFieldTypes` to be a set instead of a vector, for easy comparision of values, but that change had a lot of impacts. After that I realized I could use the `m_fieldNameVsPdxType` map for the comparision of the fields. Thinking about the order of the fields, better to compare two maps than two vectors. Next commit after this one removes this TODO
----------------------------------------------------------------
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] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<PdxType>(m_pdxType1),std::hash<PdxType>(m_pdxType2));
+ EXPECT_EQ(std::hash<PdxType>{}(m_pdxType1), std::hash<PdxType>{}(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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<PdxType>(m_pdxType1),std::hash<PdxType>(m_pdxType2));
+ EXPECT_EQ(std::hash<PdxType>{}(m_pdxType1), std::hash<PdxType>{}(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:
users@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
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441812743
##########
File path: cppcache/src/PdxTypeRegistry.hpp
##########
@@ -53,7 +53,20 @@ typedef std::unordered_map<std::shared_ptr<PdxSerializable>,
dereference_hash<std::shared_ptr<CacheableKey>>,
dereference_equal_to<std::shared_ptr<CacheableKey>>>
PreservedHashMap;
-typedef std::map<std::shared_ptr<PdxType>, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr<PdxType> const& pdx) const {
+ return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr<PdxType> const& first, std::shared_ptr<PdxType> const& second) const{
+ return first->hashcode() == second->hashcode();
+ }
+};
+
+typedef std::unordered_map<std::shared_ptr<PdxType>, int32_t, PdxTypeHashCode, PdxTypeEqualCmp>
Review comment:
+1, I like this!
----------------------------------------------------------------
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] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441805042
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector<std::shared_ptr<PdxFieldType>>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+ auto pdxPtr = *it;
+ result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
> > Just to be clear, I didn't mean to suggest we not hash in the classname at all, just outside the for loop. Also for what it's worth, even though using classname is still correct, the only instance of PDX we're aware of that has this problem, i.e. the classname isn't unique, is for JSON instances, where classname is always "__GEMFIRE_JSON".
>
> oh, I see the confusion: the classname its outside the for loop (line 573). The other classnames you are refering to are the classnames of each field. If we dont include the name of the field and its classname, the hash will be the same for an object with a bool called `foo` and for an object with a string called `foo`.
I don't think the Java implementation will distinguish one OBJECT from anther OBJECT . I think you just need to has the field name and type and ignore the class name.
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442800310
##########
File path: cppcache/integration/test/PdxJsonTypeTest.cpp
##########
@@ -142,4 +146,91 @@ TEST(PdxJsonTypeTest, testTwoConsecutiveGets) {
std::dynamic_pointer_cast<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(m_pdxType2));
Review comment:
Thanks! With this change Im finally able to verify that the tests are fine.
----------------------------------------------------------------
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] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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:
users@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #619:
URL: https://github.com/apache/geode-native/pull/619#issuecomment-647107919
> @alb3rtobr If you are struggling with formatting make sure your `clang-format` is v6. Unfortunately the versions of `clang-format` disagree on some things.
Thanks! That was the problem!
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442939224
##########
File path: cppcache/src/PdxType.hpp
##########
@@ -202,11 +202,38 @@ class PdxType : public internal::DataSerializableInternal,
bool Equals(std::shared_ptr<PdxType> otherObj);
- // This is for PdxType as key in std map.
- bool operator<(const PdxType& other) const;
+ bool operator==(const PdxType& other) const;
+
+ size_t hashcode() const;
Review comment:
Good catch, im removing 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:
users@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
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441807945
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
Review comment:
> Just a thought here. Is it necessary to hash like Java client does in this case?
No, we just need a hash function to allow identify `PdxType` object that will be assigned the same id by the server. We dont need to know the hash generated by the server.
----------------------------------------------------------------
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] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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:
users@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
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r443629523
##########
File path: cppcache/integration/benchmark/PdxTypeBM.cpp
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <benchmark/benchmark.h>
+
Review comment:
Im going to change the second include to `<framework/Cluster.h>`, maybe thats what you are referring to
----------------------------------------------------------------
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] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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:
users@infra.apache.org
[GitHub] [geode-native] alb3rtobr commented on pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #619:
URL: https://github.com/apache/geode-native/pull/619#issuecomment-647471587
I run the bechmark test I implemented as:
`$ ./cpp-benchmark --benchmark_filter=PdxTypeBM_createInstance/real_time`
The results in develop branch were:
```
-----------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------
PdxTypeBM_createInstance/real_time 69900929 ns 55007314 ns 9
```
While with the content of this PR the result was 6x faster :
```
-----------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------
PdxTypeBM_createInstance/real_time 11560270 ns 10884860 ns 50
```
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441808727
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
Review comment:
> In fact I would just create a specialization of `std::hash<PdxType>`.
I will check this
----------------------------------------------------------------
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] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441813608
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
Review comment:
See the bottom of cppcache/include/geode/CacheableKey.hpp for how we export `std::hash<CacheableKey>` and you can do the same thing for `std::hash<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:
users@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
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441827458
##########
File path: cppcache/src/PdxTypeRegistry.hpp
##########
@@ -53,7 +53,20 @@ typedef std::unordered_map<std::shared_ptr<PdxSerializable>,
dereference_hash<std::shared_ptr<CacheableKey>>,
dereference_equal_to<std::shared_ptr<CacheableKey>>>
PreservedHashMap;
-typedef std::map<std::shared_ptr<PdxType>, int32_t, PdxTypeLessThan>
+
+struct PdxTypeHashCode {
+ std::size_t operator()(std::shared_ptr<PdxType> const& pdx) const {
+ return pdx ? pdx->hashcode() : 0;
+ }
+};
+
+struct PdxTypeEqualCmp {
+ bool operator()(std::shared_ptr<PdxType> const& first, std::shared_ptr<PdxType> const& second) const{
+ return first->hashcode() == second->hashcode();
Review comment:
Sorry, I read again, I got your point, two objects could generate the same hash being different :)
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442947801
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -556,12 +556,23 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
return true;
}
-bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+bool PdxType::operator==(const PdxType& other) const {
Review comment:
I think we can get rid of `PdxType::Equals`
----------------------------------------------------------------
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] pivotal-jbarrett commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r443596357
##########
File path: cppcache/benchmark/CMakeLists.txt
##########
@@ -19,12 +19,15 @@ add_executable(cpp-benchmark
GeodeHashBM.cpp
GeodeLoggingBM.cpp
NoopBM.cpp
- )
+ PdxTypeBM.cpp)
+
+#add_dependencies(integration-framework)
target_link_libraries(cpp-benchmark
PUBLIC
apache-geode-static
benchmark::benchmark
+ integration-framework
Review comment:
Don't add this here. There is an cppcache/integration/benchmark already. Move the benchmark to the integration benchmark directory.
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442720377
##########
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:
done
##########
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<std::string> strHash;
- auto result=strHash(this->m_className);
+ auto result = strHash(this->m_className);
- for (std::vector<std::shared_ptr<PdxFieldType>>::iterator it =
- m_pdxFieldTypes->begin();
+ for (auto it = m_pdxFieldTypes->begin();
Review comment:
done
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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:
users@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
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r442947208
##########
File path: cppcache/test/CMakeLists.txt
##########
@@ -55,7 +56,7 @@ add_executable(apache-geode_unittests
util/synchronized_setTest.cpp
util/TestableRecursiveMutex.hpp
util/chrono/durationTest.cpp
-)
+ PdxTypeTest.cpp)
Review comment:
> We should check and see if `tests/cpp/testobject/PdxType.*` are in use anywhere. We may get lucky and find that they can be deleted.
It's used in several old and some new integration tests.
----------------------------------------------------------------
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] pdxcodemonkey commented on a change in pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
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<PdxInstance>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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<Properties>();
+ CacheImpl cacheImpl(&cache, properties, false, false, nullptr);
+
+ PdxTypeRegistry pdxTypeRegistry(&cacheImpl);
+
+ 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<PdxType>(m_pdxType1),std::hash<PdxType>(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:
users@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
Posted by GitBox <gi...@apache.org>.
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<std::string> strHash;
- auto result=strHash(this->m_className);
+ auto result = strHash(this->m_className);
- for (std::vector<std::shared_ptr<PdxFieldType>>::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<apache::geode::client::PdxType> {
+ 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:
users@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
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441749254
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector<std::shared_ptr<PdxFieldType>>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+ auto pdxPtr = *it;
+ result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
Just to be clear, I didn't mean to suggest we not hash in the classname at all, just outside the for loop. Also for what it's worth, even though using classname is still correct, the only instance of PDX we're aware of that has this problem, i.e. the classname isn't unique, is for JSON instances, where classname is always "__GEMFIRE_JSON".
----------------------------------------------------------------
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] pivotal-jbarrett commented on pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #619:
URL: https://github.com/apache/geode-native/pull/619#issuecomment-646662662
Good catch. I wasn’t paying attention to the fact that was in an integration test. Yes moving the hash testing to a unit test is the correct move.
----------------------------------------------------------------
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 pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #619:
URL: https://github.com/apache/geode-native/pull/619#issuecomment-647569929
Results using the new location of the bm test case:
`$ ./cpp-integration-benchmark --benchmark_filter=PdxTypeBM_createInstance/real_time`
Before this PR:
```
-----------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------
PdxTypeBM_createInstance/real_time 80933776 ns 64921880 ns 8
```
After the PR:
```
-----------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------
PdxTypeBM_createInstance/real_time 15614725 ns 14725105 ns 39
```
----------------------------------------------------------------
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 #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #619:
URL: https://github.com/apache/geode-native/pull/619#discussion_r441800944
##########
File path: cppcache/src/PdxType.cpp
##########
@@ -557,11 +557,30 @@ bool PdxType::Equals(std::shared_ptr<PdxType> otherObj) {
}
bool PdxType::operator<(const PdxType& other) const {
- auto typeIdLessThan = this->m_geodeTypeId < other.m_geodeTypeId;
- auto typeIdsBothZero =
- (this->m_geodeTypeId == 0) && (other.m_geodeTypeId == 0);
- auto classnameLessThan = this->m_className < other.m_className;
- return (typeIdLessThan || (typeIdsBothZero && classnameLessThan));
+ if (m_geodeTypeId < other.m_geodeTypeId) {
+ return true;
+ }
+
+ if ((m_geodeTypeId == 0) && (other.m_geodeTypeId == 0)) {
+ return this->m_className < other.m_className;
+ }
+
+ return false;
+}
+
+int32_t PdxType::hashcode() const {
+ std::hash<std::string> strHash;
+ auto result=strHash(this->m_className);
+
+ for (std::vector<std::shared_ptr<PdxFieldType>>::iterator it =
+ m_pdxFieldTypes->begin();
+ it != m_pdxFieldTypes->end(); ++it) {
+ auto pdxPtr = *it;
+ result = result ^ (strHash(pdxPtr->getClassName()) << 1 );
Review comment:
> Just to be clear, I didn't mean to suggest we not hash in the classname at all, just outside the for loop. Also for what it's worth, even though using classname is still correct, the only instance of PDX we're aware of that has this problem, i.e. the classname isn't unique, is for JSON instances, where classname is always "__GEMFIRE_JSON".
oh, I see the confusion: the classname its outside the for loop (line 573). The other classnames you are refering to are the classnames of each field. If we dont include the name of the field and its classname, the hash will be the same for an object with a bool called `foo` and for an object with a string called `foo`.
----------------------------------------------------------------
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] pdxcodemonkey merged pull request #619: GEODE-8212: Reduce connections to server to get type id
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #619:
URL: https://github.com/apache/geode-native/pull/619
----------------------------------------------------------------
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