isapego commented on code in PR #7080: URL: https://github.com/apache/ignite-3/pull/7080#discussion_r2589459375
########## modules/platforms/cpp/tests/fake_server/connection_test.cpp: ########## @@ -0,0 +1,73 @@ +/* +* 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 "tests/client-test/ignite_runner_suite.h" +#include "ignite/client/ignite_client.h" +#include "fake_server.h" + +#include <gtest/gtest.h> +#include <thread> + +using namespace ignite; + +class connection_test : public ignite_runner_suite { + +}; Review Comment: Minor, but let's keep it clean. ```suggestion class connection_test : public ignite_runner_suite {}; ``` ########## modules/platforms/cpp/tests/fake_server/connection_test.cpp: ########## @@ -0,0 +1,73 @@ +/* +* 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 "tests/client-test/ignite_runner_suite.h" +#include "ignite/client/ignite_client.h" +#include "fake_server.h" + +#include <gtest/gtest.h> +#include <thread> + +using namespace ignite; + +class connection_test : public ignite_runner_suite { + +}; + + +TEST_F(connection_test, handshake_with_fake_server) { Review Comment: This is the `fake_server` package, no need to make the name longer. ```suggestion TEST_F(connection_test, handshake_success) { ``` ########## modules/platforms/cpp/ignite/client/detail/node_connection.cpp: ########## @@ -203,7 +208,46 @@ std::shared_ptr<response_handler> node_connection::find_handler_unsafe(std::int6 if (it == m_request_handlers.end()) return {}; - return it->second; + return it->second.handler; +} + +void node_connection::handle_timeouts() { + auto now = std::chrono::steady_clock::now(); + + { + std::lock_guard lock(m_request_handlers_mutex); + + std::vector<int64_t> keys_for_erasure; + + for (auto& [id, req] : m_request_handlers) { + if (req.timeouts_at > now) { Review Comment: Have you pushed the changes? ########## modules/platforms/cpp/tests/fake_server/tcp_client_channel.h: ########## @@ -0,0 +1,54 @@ +/* +* 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 <atomic> +#include <cstddef> +#include <ignite/common/ignite_error.h> +#include <ignite/protocol/utils.h> +#include <netinet/in.h> +#include <sys/socket.h> +#include <unistd.h> Review Comment: Will it break release building on Windows? Let's check. It will be real problem if RE will find out about it during release procedures. ########## modules/platforms/cpp/tests/fake_server/tcp_client_channel.h: ########## @@ -0,0 +1,51 @@ +/* +* 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 <atomic> +#include <cstddef> +#include <ignite/common/ignite_error.h> +#include <ignite/protocol/utils.h> + +namespace ignite { + +/** + * Owning wrapper around server-side client socket. + */ +class tcp_client_channel { + int m_srv_fd; + int m_cl_fd = -1; + std::byte m_buf[1024]; + size_t m_pos = 0; + size_t m_remaining = 0; + std::atomic_bool m_stopped{false}; Review Comment: Let's move it to private section to keep it consistent over the code base. ########## modules/platforms/cpp/ignite/client/detail/node_connection.cpp: ########## @@ -203,7 +208,46 @@ std::shared_ptr<response_handler> node_connection::find_handler_unsafe(std::int6 if (it == m_request_handlers.end()) return {}; - return it->second; + return it->second.handler; +} + +void node_connection::handle_timeouts() { + auto now = std::chrono::steady_clock::now(); + + { + std::lock_guard lock(m_request_handlers_mutex); + + std::vector<int64_t> keys_for_erasure; Review Comment: Is it? ########## modules/platforms/cpp/tests/fake_server/connection_test.cpp: ########## @@ -0,0 +1,73 @@ +/* +* 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 "tests/client-test/ignite_runner_suite.h" +#include "ignite/client/ignite_client.h" +#include "fake_server.h" + +#include <gtest/gtest.h> +#include <thread> + +using namespace ignite; + +class connection_test : public ignite_runner_suite { + +}; + + +TEST_F(connection_test, handshake_with_fake_server) { + using namespace std::chrono_literals; + fake_server fs{}; + + fs.start(); + + ignite_client_configuration cfg; + cfg.set_logger(get_logger()); + cfg.set_endpoints({"127.0.0.1:10800"}); Review Comment: I feel like we should not use standard port number for fake server unless we want to have a lot of hard to debug flaky issues. Let's use something like `10000`, or IDK. Also, we should probably get this address from the server itself, like: ```suggestion cfg.set_endpoints(fs.get_address()); ``` ########## modules/platforms/cpp/tests/fake_server/connection_test.cpp: ########## @@ -0,0 +1,73 @@ +/* +* 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 "tests/client-test/ignite_runner_suite.h" +#include "ignite/client/ignite_client.h" +#include "fake_server.h" + +#include <gtest/gtest.h> +#include <thread> + +using namespace ignite; + +class connection_test : public ignite_runner_suite { + +}; + + +TEST_F(connection_test, handshake_with_fake_server) { + using namespace std::chrono_literals; + fake_server fs{}; + + fs.start(); + + ignite_client_configuration cfg; + cfg.set_logger(get_logger()); + cfg.set_endpoints({"127.0.0.1:10800"}); + + auto cl = ignite_client::start(cfg, 5s); +} + +TEST_F(connection_test, request_timeout) { + using namespace std::chrono_literals; Review Comment: You use `std::chrono_literals` in every function. Let's just use it globally for the file. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
