[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #630: GEODE-8102 Link and load OpenSSL library directly
pivotal-jbarrett commented on a change in pull request #630:
URL: https://github.com/apache/geode-native/pull/630#discussion_r466751787
##
File path: cppcache/src/TcpConn.cpp
##
@@ -318,14 +237,35 @@ size_t TcpConn::socketOp(TcpConn::SockOp op, char *buff,
size_t len,
return totalsend;
}
}
+ssize_t TcpConn::doOperation(const TcpConn::SockOp& op, void* buff,
+ size_t sendlen, ACE_Time_Value& waitTime,
+ size_t& readLen) const {
+ if (op == SOCK_READ) {
+return stream_->recv_n(buff, sendlen, &waitTime, &readLen);
+ } else {
+return stream_->send_n(buff, sendlen, &waitTime, &readLen);
+ }
+}
// Return the local port for this TCP connection.
uint16_t TcpConn::getPort() {
ACE_INET_Addr localAddr;
- m_io->get_local_addr(localAddr);
+ stream_->get_local_addr(localAddr);
return localAddr.get_port_number();
}
+size_t TcpConn::getDefaultChunkSize() {
+ //
+ auto pageSize = boost::interprocess::mapped_region::get_page_size();
+ if (pageSize > 1600) {
+return 1600;
+ } else if (pageSize > 0) {
+return pageSize + (1600 / pageSize) * pageSize;
+ }
+
+ return 1600;
Review comment:
I remember now why didn't make a constant. For exactly your second
question. Without an answer to that I lacked anything meaningful to name it. A
constant `constexpr auto SOME_NUMBER = 1600` felt more wrong.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #630: GEODE-8102 Link and load OpenSSL library directly
pivotal-jbarrett commented on a change in pull request #630:
URL: https://github.com/apache/geode-native/pull/630#discussion_r466751011
##
File path: cppcache/src/TcpConn.cpp
##
@@ -318,14 +237,35 @@ size_t TcpConn::socketOp(TcpConn::SockOp op, char *buff,
size_t len,
return totalsend;
}
}
+ssize_t TcpConn::doOperation(const TcpConn::SockOp& op, void* buff,
+ size_t sendlen, ACE_Time_Value& waitTime,
+ size_t& readLen) const {
+ if (op == SOCK_READ) {
+return stream_->recv_n(buff, sendlen, &waitTime, &readLen);
+ } else {
+return stream_->send_n(buff, sendlen, &waitTime, &readLen);
+ }
+}
// Return the local port for this TCP connection.
uint16_t TcpConn::getPort() {
ACE_INET_Addr localAddr;
- m_io->get_local_addr(localAddr);
+ stream_->get_local_addr(localAddr);
return localAddr.get_port_number();
}
+size_t TcpConn::getDefaultChunkSize() {
+ //
+ auto pageSize = boost::interprocess::mapped_region::get_page_size();
+ if (pageSize > 1600) {
+return 1600;
+ } else if (pageSize > 0) {
+return pageSize + (1600 / pageSize) * pageSize;
+ }
+
+ return 1600;
Review comment:
As for the constant, sure, we could add one but its really outside the
scope of the PR to make all the cleanups. Keep in mind while it appears this
code is new its actually just moved from the external file to the internal
file. GIt just can't see that.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #630: GEODE-8102 Link and load OpenSSL library directly
pivotal-jbarrett commented on a change in pull request #630:
URL: https://github.com/apache/geode-native/pull/630#discussion_r466748307
##
File path: cppcache/src/TcpConn.hpp
##
@@ -65,75 +62,33 @@ class APACHE_GEODE_EXPORT TcpConn : public Connector {
virtual void createSocket(ACE_HANDLE sock);
+ virtual ssize_t doOperation(const SockOp& op, void* buff, size_t sendlen,
+ ACE_Time_Value& waitTime, size_t& readLen) const;
+
public:
- size_t m_chunkSize;
-
- static size_t getDefaultChunkSize() {
-// Attempt to set chunk size to nearest OS page size
-// for perf improvement
-auto pageSize = boost::interprocess::mapped_region::get_page_size();
-if (pageSize > 1600) {
- return 1600;
-} else if (pageSize > 0) {
- return pageSize + (1600 / pageSize) * pageSize;
-}
-
-return 1600;
- }
-
- TcpConn(const char* hostname, int32_t port,
+ TcpConn(const std::string& hostname, uint16_t port,
std::chrono::microseconds waitSeconds, int32_t maxBuffSizePool);
- TcpConn(const char* ipaddr, std::chrono::microseconds waitSeconds,
+
+ TcpConn(const std::string& address, std::chrono::microseconds waitSeconds,
int32_t maxBuffSizePool);
- virtual ~TcpConn() override { close(); }
+ ~TcpConn() override {}
Review comment:
Because close is a virtual function and calling virtual functions from
destructors is undefined.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #630: GEODE-8102 Link and load OpenSSL library directly
pivotal-jbarrett commented on a change in pull request #630:
URL: https://github.com/apache/geode-native/pull/630#discussion_r466748099
##
File path: cppcache/src/TcpConn.cpp
##
@@ -318,14 +237,35 @@ size_t TcpConn::socketOp(TcpConn::SockOp op, char *buff,
size_t len,
return totalsend;
}
}
+ssize_t TcpConn::doOperation(const TcpConn::SockOp& op, void* buff,
+ size_t sendlen, ACE_Time_Value& waitTime,
+ size_t& readLen) const {
+ if (op == SOCK_READ) {
+return stream_->recv_n(buff, sendlen, &waitTime, &readLen);
+ } else {
+return stream_->send_n(buff, sendlen, &waitTime, &readLen);
+ }
+}
// Return the local port for this TCP connection.
uint16_t TcpConn::getPort() {
ACE_INET_Addr localAddr;
- m_io->get_local_addr(localAddr);
+ stream_->get_local_addr(localAddr);
return localAddr.get_port_number();
}
+size_t TcpConn::getDefaultChunkSize() {
+ //
+ auto pageSize = boost::interprocess::mapped_region::get_page_size();
+ if (pageSize > 1600) {
+return 1600;
+ } else if (pageSize > 0) {
+return pageSize + (1600 / pageSize) * pageSize;
+ }
+
+ return 1600;
Review comment:
Yeah, I didn't want to mess with it. We could through the comment back
in but I felt it was an inaccurate statement. Clearly it was picking something
at the os page size given the method name it was calling, but the other magic
going on didn't. Lacking interest to write a benchmark and test around the
change to pull it I just left the code.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #630: GEODE-8102 Link and load OpenSSL library directly
pivotal-jbarrett commented on a change in pull request #630:
URL: https://github.com/apache/geode-native/pull/630#discussion_r463914946
##
File path: clicache/src/CMakeLists.txt
##
@@ -23,7 +23,7 @@
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/impl/AssemblyInfo.cpp.in ${CMAKE_CURR
list(APPEND CONFIGURE_IN_FILES
${CMAKE_CURRENT_SOURCE_DIR}/impl/AssemblyInfo.cpp.in)
list(APPEND CONFIGURE_OUT_FILES
${CMAKE_CURRENT_BINARY_DIR}/impl/AssemblyInfo.cpp)
-add_library(${PROJECT_NAME} SHARED
+add_library(Apache.Geode SHARED
Review comment:
Most of the IDE's that integrate with CMake don't resolve the target
names correctly if they are variables. It's also not necessary that all targets
be unique projects in CMake. It was a bad pattern that was used in the early
days in CMake and in Geode Native (my bad).
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
