[
https://issues.apache.org/jira/browse/GEODE-10300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17564394#comment-17564394
]
ASF GitHub Bot commented on GEODE-10300:
----------------------------------------
pivotal-jbarrett commented on code in PR #970:
URL: https://github.com/apache/geode-native/pull/970#discussion_r917042197
##########
cppcache/src/GetAllServersResponse.cpp:
##########
@@ -21,10 +21,10 @@ namespace geode {
namespace client {
void GetAllServersResponse::toData(DataOutput& output) const {
- int32_t numServers = static_cast<int32_t>(m_servers.size());
+ int32_t numServers = static_cast<int32_t>(servers_.size());
Review Comment:
`auto`
##########
cppcache/include/geode/DataInput.hpp:
##########
@@ -502,21 +502,21 @@ class APACHE_GEODE_EXPORT DataInput {
inline char readPdxChar() { return static_cast<char>(readInt16()); }
- inline void _checkBufferSize(size_t size, int32_t line) {
- if ((m_bufLength - (m_buf - m_bufHead)) < size) {
+ virtual void _checkBufferSize(size_t size, int32_t line) {
Review Comment:
This is a breaking ABI change. This should only occur at a minor release.
Are we sure this needs to change?
##########
cppcache/src/StreamDataInput.cpp:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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;
Review Comment:
`constexpr`
##########
cppcache/test/StreamDataInputTest.cpp:
##########
@@ -0,0 +1,139 @@
+/*
+ * 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 <gmock/gmock-actions.h>
+#include <gmock/gmock-matchers.h>
+
+#include <gtest/gtest.h>
+
+#include "CacheImpl.hpp"
+#include "Connector.hpp"
+#include "GetAllServersResponse.hpp"
+#include "ServerLocation.hpp"
+#include "StreamDataInput.hpp"
+#include "geode/DataOutput.hpp"
+#include "mock/ConnectorMock.hpp"
+
+namespace {
+
+using apache::geode::client::CacheImpl;
+using apache::geode::client::Connector;
+using apache::geode::client::ConnectorMock;
+using apache::geode::client::DataOutput;
+using apache::geode::client::GetAllServersResponse;
+using apache::geode::client::Serializable;
+using apache::geode::client::ServerLocation;
+using apache::geode::client::StreamDataInput;
+using apache::geode::client::TimeoutException;
+
+using ::testing::_;
+using ::testing::DoAll;
+using ::testing::Eq;
+using ::testing::Return;
+using ::testing::SetArrayArgument;
+using ::testing::SizeIs;
+
+const size_t kReadBuffSize = 3000;
Review Comment:
`constexpr`
##########
cppcache/include/geode/DataInput.hpp:
##########
@@ -452,19 +452,19 @@ class APACHE_GEODE_EXPORT DataInput {
DataInput& operator=(DataInput&&) = default;
protected:
+ const uint8_t* buf_;
Review Comment:
Love the update to the current naming convention but please also update to
full words too.
##########
cppcache/src/StreamDataInput.cpp:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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),
+ connector_(std::move(connector)),
+ remainingTimeBeforeTimeout_(timeout) {}
+
+void StreamDataInput::readDataIfNotAvailable(size_t size) {
+ char buff[kBufferSize];
+ while (getBytesRemaining() < size) {
+ const auto start = std::chrono::system_clock::now();
+
+ const auto receivedLength = connector_->receive_nothrowiftimeout(
+ buff, kBufferSize,
+ std::chrono::duration_cast<std::chrono::milliseconds>(
+ remainingTimeBeforeTimeout_));
Review Comment:
If the most granular time is actually in milliseconds does it make sense to
even have `remainingTimeBeforeTimeout_` be in microseconds?
##########
cppcache/include/geode/DataInput.hpp:
##########
@@ -452,19 +452,19 @@ class APACHE_GEODE_EXPORT DataInput {
DataInput& operator=(DataInput&&) = default;
protected:
Review Comment:
Making all these protected is likely an ABI breaking change.
> 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: 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.10#820010)
