[ 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)