[
https://issues.apache.org/jira/browse/GEODE-10300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17536517#comment-17536517
]
ASF GitHub Bot commented on GEODE-10300:
----------------------------------------
gaussianrecurrence commented on code in PR #970:
URL: https://github.com/apache/geode-native/pull/970#discussion_r872129402
##########
cppcache/include/geode/DataInput.hpp:
##########
@@ -452,19 +452,19 @@ class APACHE_GEODE_EXPORT DataInput {
DataInput& operator=(DataInput&&) = default;
protected:
+ const uint8_t* m_buf;
+ const uint8_t* m_bufHead;
Review Comment:
Naming notation for member attributes in Geode Native follows lowerCamelCase
with '_' as a postfix, to indicate that it's a member variable. As example
'm_buf' would rather be 'buf_'. Usually whenever we make a change that touches
part of the code with the old notation (m_...) the idea is to change it to the
right notation
##########
cppcache/src/GetAllServersResponse.hpp:
##########
@@ -42,6 +42,11 @@ class GetAllServersResponse : public
internal::DataSerializableFixedId_t<
return std::make_shared<GetAllServersResponse>();
}
GetAllServersResponse() : Serializable() {}
+ explicit GetAllServersResponse(
+ std::vector<std::shared_ptr<ServerLocation> > servers)
+ : Serializable() {
+ m_servers = servers;
Review Comment:
Same comment regarding member attributes naming conventions:
- Please use lowerCamelCase_ notation.
- All the member attributes should be declared at the end of the class
rather than at the beginning.
Also, I've seen member attributes scope is not explicitly set in this class,
by default this means it would be private, but I usually like to explicitly
state it. So don't forget to state the scope after chaning the location of the
member attributes :)
##########
cppcache/src/GetAllServersResponse.hpp:
##########
@@ -42,6 +42,11 @@ class GetAllServersResponse : public
internal::DataSerializableFixedId_t<
return std::make_shared<GetAllServersResponse>();
}
GetAllServersResponse() : Serializable() {}
+ explicit GetAllServersResponse(
+ std::vector<std::shared_ptr<ServerLocation> > servers)
+ : Serializable() {
+ m_servers = servers;
Review Comment:
This is only used for testing, however if you pass an object copy to this
constructor and then make a copy-assign, you'd be copying this twice. Please
use std::move :)
##########
cppcache/src/GetAllServersResponse.hpp:
##########
@@ -42,6 +42,11 @@ class GetAllServersResponse : public
internal::DataSerializableFixedId_t<
return std::make_shared<GetAllServersResponse>();
}
GetAllServersResponse() : Serializable() {}
+ explicit GetAllServersResponse(
+ std::vector<std::shared_ptr<ServerLocation> > servers)
+ : Serializable() {
+ m_servers = servers;
Review Comment:
Usually it's preferred, when possible to use constructor inline
initialization for member attributes
##########
cppcache/src/StreamDataInput.cpp:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 "StreamDataInput.hpp"
+
+#include <geode/DataInput.hpp>
+
+#include "Utils.hpp"
+#include "util/Log.hpp"
+
+namespace apache {
+namespace geode {
+namespace client {
+
+const size_t kBufferSize = 3000;
+
+StreamDataInput::StreamDataInput(std::chrono::milliseconds timeout,
+ std::unique_ptr<Connector> connector,
+ const CacheImpl* cache, Pool* pool)
+ : DataInput(nullptr, 0, cache, pool) {
+ m_remainingTimeBeforeTimeout = timeout;
Review Comment:
Same comment regarding member attributes naming conventions:
- Please use lowerCamelCase_ notation.
- Also, when possible, use constructor inline initialization for member
attributes.
##########
cppcache/src/StreamDataInput.cpp:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 "StreamDataInput.hpp"
+
+#include <geode/DataInput.hpp>
+
+#include "Utils.hpp"
+#include "util/Log.hpp"
+
+namespace apache {
+namespace geode {
+namespace client {
+
+const size_t kBufferSize = 3000;
+
+StreamDataInput::StreamDataInput(std::chrono::milliseconds timeout,
+ std::unique_ptr<Connector> connector,
+ const CacheImpl* cache, Pool* pool)
+ : DataInput(nullptr, 0, cache, pool) {
+ m_remainingTimeBeforeTimeout = timeout;
+ m_connector = std::move(connector);
+ m_buf = nullptr;
+ m_bufHead = m_buf;
+ m_bufLength = 0;
+}
+
+StreamDataInput::~StreamDataInput() {
+ if (m_bufHead != nullptr) {
+ free(const_cast<uint8_t*>(m_bufHead));
+ }
+}
+
+void StreamDataInput::readDataIfNotAvailable(size_t size) {
+ char buff[kBufferSize];
+ while ((m_bufLength - (m_buf - m_bufHead)) < size) {
Review Comment:
getBytesRemaining gives you exactly the value of (m_bufLength - (m_buf -
m_bufHead)), so considering it provides you with a richer semantical meaning,
and that it would probably be inlined by the compiler, I'd use that function
instead.
##########
cppcache/src/StreamDataInput.cpp:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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 "StreamDataInput.hpp"
+
+#include <geode/DataInput.hpp>
+
+#include "Utils.hpp"
+#include "util/Log.hpp"
+
+namespace apache {
+namespace geode {
+namespace client {
+
+const size_t BUFF_SIZE = 3000;
+
+StreamDataInput::StreamDataInput(std::chrono::milliseconds timeout,
+ std::unique_ptr<Connector> connector,
+ const CacheImpl* cache, Pool* pool)
+ : DataInput(nullptr, 0, cache, pool) {
+ m_remainingTimeBeforeTimeout = timeout;
+ m_connector = std::move(connector);
+ m_buf = nullptr;
+ m_bufHead = m_buf;
+ m_bufLength = 0;
+}
+
+StreamDataInput::~StreamDataInput() {
+ if (m_bufHead != nullptr) {
+ free(const_cast<uint8_t*>(m_bufHead));
Review Comment:
One feasible solution would be to have an std::vector<uint8_t> as buffer for
StreamDataInput, this way you don't need to perform memory managment, that is
prone to mem leaks.
> C++ Native client messages coming from the locator cannot be longer than 3000
> bytes
> -----------------------------------------------------------------------------------
>
> Key: GEODE-10300
> URL: https://issues.apache.org/jira/browse/GEODE-10300
> Project: Geode
> Issue Type: Bug
> Components: native client
> Reporter: Alberto Gomez
> Assignee: Alberto Gomez
> Priority: Major
> Labels: needsTriage, pull-request-available
>
> If a locator sends a response to the C++ native client that is longer than
> 3000 bytes the C++ native client library will only read the first 3000 bytes.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
