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 2022/01/15 14:20:38 UTC
[GitHub] [geode-native] mmartell opened a new pull request #909: GEODE-9712: genericize netcore
mmartell opened a new pull request #909:
URL: https://github.com/apache/geode-native/pull/909
This brings in support for our existing IRegion<TKey, TValue> interface with the following restrictions:
TKey currently limited to string, short, and int.
TValue currently limited to string only.
SessionState tests currently failing. Not sure why yet.
Note:
Leaving as draft at least until SessionState issue figured out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
mmartell commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1030084621
With the decision to go with a pure C# client for .NET, we no loner need this interop based approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on a change in pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787266693
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
Review comment:
Ideally, the C# layer would have access to the DataInput and DataOutput buffers via the Region. However, this would likely require moving too much buffer management code to C#. An alternative that might be worth consideration is to have an interop buffer in the C++ heap (an std::vector), and give C# its address. This could be created during the cache create and deleted when the cache is closed. Always passing the length(s) of interop parameters would allow growing the buffer as needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787987937
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
+ hashCode_ = internal::geode_hash<std::vector<int8_t>>{}(bytes_);
Review comment:
@pivotal-jbarrett I think I need the short course in what we're using `hashcode` for that causes issues here. I wasn't aware that we were comparing or otherwise using server-side hashes against client-side, which... sounds like a bad idea to begin with, but I have to admit I'm not surprised by.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1013900843
This pull request **introduces 6 alerts** when merging 62c1d0a2f3a9b6e6c30a4a23f1d498ccaa43ca5d into 44abff89f65f9c1bc72f504f3d57800dd1e48cbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-f91dc2dae3292c4cb8d6fd25571b6a175b02e173)
**new alerts:**
* 3 for Useless assignment to local variable
* 3 for Empty branch of conditional, or empty loop body
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
mmartell commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1013690549
Please disregard my previous PR for GEODE-9712. It had merge conflicts due to erroneous branch being created on apache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
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 #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787975269
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
+ hashCode_ = internal::geode_hash<std::vector<int8_t>>{}(bytes_);
Review comment:
The probably with `CacheableKey` is that it expects an equality rule that isn't necessary at the serialization layer but is at the local cache layer. The C-binding is going to need to provide a function pointer, or something I can't think of right now, to solve that local caching key issue. Value objects don't need hash or equality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
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 #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787997787
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
+ hashCode_ = internal::geode_hash<std::vector<int8_t>>{}(bytes_);
Review comment:
The hash is used to route the keys to the correct server. If the client and server don't agree on the hashing algorithm and result then they will silently work but it results in suboptimal operations with multiple hops.
Equals is only going to be used client side for local caching, if caching is enabled. Hash maps require both a hash and equality function. The equality on the client and server don't have to match but given the hash needs to match it silly not to have the equality match. One problem that could exist if you don't use constant equality is that the server and client could disagree on what keys are in the caches. Imagine an object that has a field that is serialized, like timestamp, that isn't part of the equality or hash because its ephemeral but we hash the serialized form then the server and client won't agree on the hash value. Then if caching was enabled and equality is also based on the serialized form then the sever would only have one Key('foo') with some timestamp value X and the client could have Key('foo') with timestamp Y and Key('foo') with timestamp X. Local cache lookups would fail as timestamp advances, which could cause massive heap exhaustion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell closed pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
mmartell closed pull request #909:
URL: https://github.com/apache/geode-native/pull/909
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
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 #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787112789
##########
File path: c-bindings/include/geode/region.h
##########
@@ -40,18 +40,41 @@ APACHE_GEODE_C_EXPORT void apache_geode_Region_PutString(
apache_geode_region_t* region, const char* key, const char* value);
APACHE_GEODE_C_EXPORT void apache_geode_Region_PutByteArray(
- apache_geode_region_t* region, const char* key, const char* value, size_t size);
+ apache_geode_region_t* region, const char* key, size_t keyLength,
Review comment:
We should really require length parameters for all array pointers, it's not safe to rely on `NULL` termination.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on a change in pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787267405
##########
File path: c-bindings/src/data_serializable_raw.hpp
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <vector>
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+class DataSerializableRaw : public DataSerializablePrimitive , public CacheableKey {
+ public:
+ DataSerializableRaw(const int8_t* data, size_t size);
+ ~DataSerializableRaw() noexcept override = default;
+
+ static std::shared_ptr<DataSerializableRaw> create(
+ const int8_t* data, size_t size);
+
+
+ virtual void toData(DataOutput& dataOutput) const override;
Review comment:
Good catch! I guess you're better than clang-tidy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1014190356
This pull request **introduces 10 alerts** when merging d7b60c0d2cc3d7f2399c20f5491713115ad007b0 into 44abff89f65f9c1bc72f504f3d57800dd1e48cbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-c6516fab928721b78b550f68df4eeaa879a52a62)
**new alerts:**
* 7 for Useless assignment to local variable
* 2 for Empty branch of conditional, or empty loop body
* 1 for Dereferenced variable may be null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on a change in pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787248431
##########
File path: c-bindings/include/geode/region.h
##########
@@ -40,18 +40,41 @@ APACHE_GEODE_C_EXPORT void apache_geode_Region_PutString(
apache_geode_region_t* region, const char* key, const char* value);
APACHE_GEODE_C_EXPORT void apache_geode_Region_PutByteArray(
- apache_geode_region_t* region, const char* key, const char* value, size_t size);
+ apache_geode_region_t* region, const char* key, size_t keyLength,
Review comment:
I don't disagree. However, I'm thinking we should consider the single more powerful apache_geode_Region_Put (line 50), which includes lengths for both key and value, and do away with all other versions of the Put API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787984814
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
Review comment:
To an extent this is a "six of one and half a dozen of another" issue, but I suppose it is a little bit cleaner to just go with one parameter of type `std::vector` in the ctor, then use `std:move` move to copy it into bytes_.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1013823849
This pull request **introduces 10 alerts** when merging 343cdb0ff98df1caeb809461fd9383c16a3e02cf into 44abff89f65f9c1bc72f504f3d57800dd1e48cbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-b241246c9e37b9a25de60c5d88688d9b9e4d36eb)
**new alerts:**
* 4 for Dereferenced variable may be null
* 3 for Useless assignment to local variable
* 3 for Empty branch of conditional, or empty loop body
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
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 #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787172287
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
+ hashCode_ = internal::geode_hash<std::vector<int8_t>>{}(bytes_);
Review comment:
This is going to result in a hash on the client that is not consistent with the hash on the server. The hash value needs to come from the pre-serialized form of the object being serialized here. This will result in sub-optimal put operations.
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
Review comment:
Any way to use `std::vector`? In other places where we needed arrays of bytes we have used `std::vector` to encapsulate the length and lifecycle management of the array. While in this method the `const` makes it very clear that the caller is the owner often times it isn't so clear.
##########
File path: c-bindings/src/data_serializable_raw.hpp
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <vector>
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+class DataSerializableRaw : public DataSerializablePrimitive , public CacheableKey {
+ public:
+ DataSerializableRaw(const int8_t* data, size_t size);
+ ~DataSerializableRaw() noexcept override = default;
+
+ static std::shared_ptr<DataSerializableRaw> create(
+ const int8_t* data, size_t size);
+
+
+ virtual void toData(DataOutput& dataOutput) const override;
Review comment:
`virtual` is implied by `override`. `clang-tidy` should be complaining about this.
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
Review comment:
`0` is a valid hash so this could result in multiple re-hashes.
##########
File path: c-bindings/src/data_serializable_raw.hpp
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <vector>
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+class DataSerializableRaw : public DataSerializablePrimitive , public CacheableKey {
+ public:
+ DataSerializableRaw(const int8_t* data, size_t size);
+ ~DataSerializableRaw() noexcept override = default;
+
+ static std::shared_ptr<DataSerializableRaw> create(
+ const int8_t* data, size_t size);
+
+
+ virtual void toData(DataOutput& dataOutput) const override;
+ virtual void fromData(DataInput& dataInput) override;
+ virtual DSCode getDsCode() const override;
+
+ bool operator==(const CacheableKey& other) const override;
+
+ int32_t hashcode() const override;
+
+ private:
+ std::vector<int8_t> bytes_;
+ static constexpr int dsCodeSize_ = 1;
Review comment:
Constants have a different naming convention.
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
+ hashCode_ = internal::geode_hash<std::vector<int8_t>>{}(bytes_);
+ }
+ return 0;
+}
+
+} // namespace internal
+} // namespace client
+} // namespace geode
+} // namespace apache
Review comment:
Missing new line.
##########
File path: c-bindings/src/data_serializable_raw.hpp
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
Review comment:
Missing header guard. `#pragma once` is not a standard.
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
Review comment:
The corresponding header for a class should always be listed first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r788002347
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
+ hashCode_ = internal::geode_hash<std::vector<int8_t>>{}(bytes_);
Review comment:
Okay, so the `DataSerializableRaw` hack is essentially useless, is what I hear you saying, because without a way to implement `hashcode` and `==` we can only use it for values. Probably time for a new plan for keys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1013704419
This pull request **introduces 6 alerts** when merging ce2e194772d2de913cd7b8c487510473d0f22874 into 44abff89f65f9c1bc72f504f3d57800dd1e48cbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-bfd9f12933341d29ac7cee7391afdd4b045a2c0f)
**new alerts:**
* 3 for Useless assignment to local variable
* 3 for Empty branch of conditional, or empty loop body
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1013691660
This pull request **introduces 10 alerts** when merging 5462645b9af941be1e8612a85ef93e97019c53a6 into 44abff89f65f9c1bc72f504f3d57800dd1e48cbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-75546344dca449665e84a95046bef04eb8c7a7cb)
**new alerts:**
* 4 for Dereferenced variable may be null
* 3 for Useless assignment to local variable
* 3 for Empty branch of conditional, or empty loop body
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1013776312
This pull request **introduces 6 alerts** when merging 34f6465746405dbd5ce4f8c51624d37e0761d7dd into 44abff89f65f9c1bc72f504f3d57800dd1e48cbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-1c90cccca93b609e5ed4815f0dd5e7183759b2cb)
**new alerts:**
* 3 for Useless assignment to local variable
* 3 for Empty branch of conditional, or empty loop body
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on a change in pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787971403
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
+ hashCode_ = internal::geode_hash<std::vector<int8_t>>{}(bytes_);
Review comment:
Good catch! I think we're going to have to create the CacheableKey objects at the C_Bindings layer, similar to what clicache does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
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 #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r788003367
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
+ hashCode_ = internal::geode_hash<std::vector<int8_t>>{}(bytes_);
Review comment:
Yes, and for values you don't need 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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1013753410
This pull request **introduces 6 alerts** when merging deca4b7c87ba4a5844e2222e6db4443782d098dc into 44abff89f65f9c1bc72f504f3d57800dd1e48cbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-dc3e10bd105d0fe37c7f51207bcfcb623d92b13f)
**new alerts:**
* 3 for Useless assignment to local variable
* 3 for Empty branch of conditional, or empty loop body
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on pull request #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #909:
URL: https://github.com/apache/geode-native/pull/909#issuecomment-1015816463
This pull request **introduces 10 alerts** when merging 0197951911cd3b86f556920fbb14c2672f8d1c52 into 0550b0ff4d4040500cc52baef9297c01691011a8 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-a6b3342ad72ded4afd2a579216a6799d5c6120b6)
**new alerts:**
* 7 for Useless assignment to local variable
* 2 for Empty branch of conditional, or empty loop body
* 1 for Dereferenced variable may be null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
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 #909: GEODE-9712: genericize netcore
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #909:
URL: https://github.com/apache/geode-native/pull/909#discussion_r787984684
##########
File path: c-bindings/src/data_serializable_raw.cpp
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 <iterator>
+
+// C++ client public headers
+#include "geode/DataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "geode/internal/DataSerializablePrimitive.hpp"
+
+// C client public headers
+#include "data_serializable_raw.hpp"
+
+// C client private headers
+
+using apache::geode::client::DataInput;
+using apache::geode::client::DataOutput;
+
+namespace apache {
+namespace geode {
+namespace client {
+namespace internal {
+
+DataSerializableRaw::DataSerializableRaw(const int8_t* data, size_t size) {
+ bytes_.reserve(size);
+ std::copy(data, data + size, std::back_inserter(bytes_));
+}
+
+std::shared_ptr<DataSerializableRaw> DataSerializableRaw::create(
+ const int8_t* data, size_t size) {
+ return std::make_shared<DataSerializableRaw>(data, size);
+}
+
+void DataSerializableRaw::toData(DataOutput& dataOutput) const {
+ dataOutput.writeBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+void DataSerializableRaw::fromData(DataInput& dataInput) {
+ dataInput.readBytesOnly(bytes_.data() + dsCodeSize_,
+ bytes_.size() - dsCodeSize_);
+}
+
+DSCode DataSerializableRaw::getDsCode() const {
+ return static_cast<DSCode>(bytes_[0]);
+}
+
+bool DataSerializableRaw::operator==(const CacheableKey& other) const {
+ if (auto otherKey = dynamic_cast<const DataSerializableRaw*>(&other)) {
+ return bytes_ == otherKey->bytes_;
+ }
+
+ return false;
+}
+
+int32_t DataSerializableRaw::hashcode() const {
+ if (hashCode_ == 0) {
+ hashCode_ = internal::geode_hash<std::vector<int8_t>>{}(bytes_);
Review comment:
When I thought about this problem years ago I felt that the C-binding layer would either need to below the local caching and serialization layers and effectively be a networking layer, or the C-binding will need to keep track of pointers back to the original C types and some function pointers. Neither of them are going to be easy.
```
class VoidCachableKey {
public void VoidCacheableKey(const void* key, int32_t (*hash)(const void *), bool (*equals)(const void *, const void *, const char* serialized, size_t serializedLength) : ... {}
int32_t hashcode() {
return hash_(key);
}
bool equals(const VoidCachableKey& other) {
return equals_(key, other.key_);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org