[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, , );
+ } else {
+return stream_->send_n(buff, sendlen, , );
+ }
+}
// 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_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, , );
+ } else {
+return stream_->send_n(buff, sendlen, , );
+ }
+}
// 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] bschuchardt merged pull request #5422: GEODE-6950: hot loop in PrimaryHandler.processRequest()
bschuchardt merged pull request #5422: URL: https://github.com/apache/geode/pull/5422 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] dschneider-pivotal opened a new pull request #5431: change memberDeparted to disconnect the connection
dschneider-pivotal opened a new pull request #5431: URL: https://github.com/apache/geode/pull/5431 Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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] pdxcodemonkey commented on a change in pull request #630: GEODE-8102 Link and load OpenSSL library directly
pdxcodemonkey commented on a change in pull request #630:
URL: https://github.com/apache/geode-native/pull/630#discussion_r466728135
##
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, , );
+ } else {
+return stream_->send_n(buff, sendlen, , );
+ }
+}
// 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:
Can I get a named constant for this, please? Do you know what
significance the number 16 million has here? Also, isn't (1600 / pageSize)
* pageSize just always 1600, so we're just adding 1600 to pageSize on
line 263?
##
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, , );
+ } else {
+return stream_->send_n(buff, sendlen, , );
+ }
+}
// 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:
So line 263 is some misguided attempt to use integer division to our
advantage, but I'm pretty sure it's wrong, unless for some reason we know the
Geode server is doing the exact same bizarre calculation and we need to
replicate it. What the heck is this used for?
##
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:
Why lose the call to `close()` here? Is it called in the base dtor?
##
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, , );
+ } else {
+return stream_->send_n(buff, sendlen, , );
+ }
+}
// 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
[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_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, , );
+ } else {
+return stream_->send_n(buff, sendlen, , );
+ }
+}
// 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] agingade commented on a change in pull request #5424: GEODE-8394: Rewind the message Part on command failure
agingade commented on a change in pull request #5424:
URL: https://github.com/apache/geode/pull/5424#discussion_r466584837
##
File path:
geode-core/src/distributedTest/java/org/apache/geode/cache30/ClientServerCacheOperationDUnitTest.java
##
@@ -0,0 +1,200 @@
+/*
+ * 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.
+ */
+package org.apache.geode.cache30;
+
+import static org.apache.geode.cache.RegionShortcut.REPLICATE;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.RegionFactory;
+import org.apache.geode.cache.client.ClientRegionShortcut;
+import org.apache.geode.cache.client.Pool;
+import org.apache.geode.cache.client.PoolManager;
+import org.apache.geode.cache.client.ServerConnectivityException;
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.test.dunit.DistributedTestUtils;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.ClientCacheRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+import org.apache.geode.test.junit.categories.ClientServerTest;
+
+@Category({ClientServerTest.class})
+public class ClientServerCacheOperationDUnitTest implements Serializable {
Review comment:
It was the test used to reproduce the original issue and test the fix.
If you can see it has dependency on environment and shows flaky-ness...With
adjusting the time; it is more of becoming a test for large object.
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] jivthesh opened a new pull request #5430: added code of conduct to project
jivthesh opened a new pull request #5430: URL: https://github.com/apache/geode/pull/5430 Thank you for submitting a contribution to Apache Geode. In order to streamline the review of the contribution we ask you to ensure the following steps have been taken: ### For all changes: - [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message? - [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)? - [ ] Is your initial contribution a single, squashed commit? - [ ] Does `gradlew build` run cleanly? - [ ] Have you written or updated unit tests to verify your changes? - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? ### Note: Please ensure that once the PR is submitted, check Concourse for build issues and submit an update to your PR as soon as possible. If you need help, please send an email to d...@geode.apache.org. 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] Bill commented on a change in pull request #5422: GEODE-6950: hot loop in PrimaryHandler.processRequest()
Bill commented on a change in pull request #5422:
URL: https://github.com/apache/geode/pull/5422#discussion_r466543551
##
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/locator/PrimaryHandler.java
##
@@ -39,9 +39,17 @@
private TcpServer tcpServer;
private int locatorWaitTime;
- PrimaryHandler(TcpHandler fallbackHandler, int locatorWaitTime) {
+ @FunctionalInterface
+ interface Sleeper {
+void sleep(long msToSleep) throws InterruptedException;
+ }
+
+ private Sleeper sleeper;
+
+ PrimaryHandler(TcpHandler fallbackHandler, int locatorWaitTime, Sleeper
sleeper) {
Review comment:
Injecting the sleeper is a good idea
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] bschuchardt merged pull request #5428: GEODE-8407: MergeLogFiles fails to include files with the same name b…
bschuchardt merged pull request #5428: URL: https://github.com/apache/geode/pull/5428 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] lgtm-com[bot] commented on pull request #5348: DRAFT: Add flag in gateway sender to not store dropped events while stopped …
lgtm-com[bot] commented on pull request #5348: URL: https://github.com/apache/geode/pull/5348#issuecomment-669908975 This pull request **introduces 1 alert** when merging 3f749fa776d4f79a637e1851a8c23ceb3eec2565 into eb8668ad7caa6e20d9be242ad719e173ebf244d8 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-2df4e08551c8b1c8cda7cb0c66d0fbc8a15697e4) **new alerts:** * 1 for Dereferenced variable may be null 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] albertogpz commented on a change in pull request #5348: DRAFT: Add flag in gateway sender to not store dropped events while stopped …
albertogpz commented on a change in pull request #5348: URL: https://github.com/apache/geode/pull/5348#discussion_r466370988 ## File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/StartGatewaySenderCommand.java ## @@ -155,4 +160,38 @@ public ResultModel startGatewaySender(@CliOption(key = CliStrings.START_GATEWAYS return resultModel; } + + private boolean sendAllStartingMessage(Set dsMembers, Cache cache, Review comment: I have added a new commit with a change to function invocation instead of doing it through JMX. 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] alb3rtobr commented on pull request #628: GEODE-8344: Add GatewaySenderEventCallbackArgument class
alb3rtobr commented on pull request #628:
URL: https://github.com/apache/geode-native/pull/628#issuecomment-669938261
With the new integration test it is possible to reproduce the problem and
ensure it is solved.
These two logs messages can be found on test log:
```
Region::put: region [/region] created key [order], value [OrderID: 2 Product
Name: product y Quantity: 37]
Region::put: region [/region] created key [order], value
[PDX[28743191,com.example.Order]{name=product y,order_id=2,quantity=37}]
```
Without the GatewaySenderEventCallbackArgument class added to the code,
there is a deserialization exception when receiving the second event, so
instead of that message, this is one appears:
```
Exception while receiving subscription event for endpoint localhost:33829::
apache::geode::client::IllegalStateException: Unregistered type in
deserialization
```
As the event generated in cluster A is not deserialized correctly in cluster
B, it is not processed by the dummy cache listener implemented, and the test
case will fail with this error:
```
Expected equality of these values:
cacheListenerB->getNumEvents()
Which is: 0
1
```
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
