You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by gregt5259 <gi...@git.apache.org> on 2017/07/02 10:26:45 UTC
[GitHub] geode-native pull request #105: GEODE-2891 connect-timeout violation in C++ ...
GitHub user gregt5259 opened a pull request:
https://github.com/apache/geode-native/pull/105
GEODE-2891 connect-timeout violation in C++ Native Client
THe fix enables to interpret the time measure unit in handshake as milliseconds rather than seconds.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gregt5259/geode-native develop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode-native/pull/105.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #105
----
commit d4c84019fc225a9a83f8a23f127ccc6fcddf4146
Author: gregt5259 <gr...@amdocs.com>
Date: 2017-07-02T07:27:50Z
Merge remote-tracking branch 'refs/remotes/apache/develop' into develop
commit c5743fea51b390338f9cd548b5aef3f888c6ce0a
Author: gregt5259 <gr...@amdocs.com>
Date: 2017-07-02T10:16:26Z
GEODE-2891: connect-timeout violation in C++ Native Client
Change time measure unit for handshake from seconds to milliseconds
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #105: GEODE-2891 connect-timeout violation in C++ ...
Posted by gregt5259 <gi...@git.apache.org>.
Github user gregt5259 closed the pull request at:
https://github.com/apache/geode-native/pull/105
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #105: GEODE-2891 connect-timeout violation in C++ ...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/105#discussion_r125184917
--- Diff: src/cppcache/src/TcrConnection.cpp ---
@@ -703,15 +717,21 @@ inline ConnErrType TcrConnection::sendData(uint32_t& timeSpent,
is not public yet*/
notPublicApiWithTimeout == TcrMessage::EXECUTE_FUNCTION ||
notPublicApiWithTimeout == TcrMessage::EXECUTE_REGION_FUNCTION ||
- notPublicApiWithTimeout ==
- TcrMessage::EXECUTE_REGION_FUNCTION_SINGLE_HOP) {
- // then app has set timeout in millis, change it to microSeconds
- sendTimeoutSec = sendTimeoutSec * 1000;
- isPublicApiTimeout = true;
- LOGDEBUG("sendData2 %d ", sendTimeoutSec);
- } else {
- sendTimeoutSec = sendTimeoutSec * 1000;
- }
+ notPublicApiWithTimeout == TcrMessage::EXECUTE_REGION_FUNCTION_SINGLE_HOP ||
+ // GT GEODE-2891
+ notPublicApiWithTimeout == TcrMessage::HANDSHAKE)
+ {
+ // then app has set timeout in millis, change it to microSeconds
+ sendTimeoutSec = sendTimeoutSec * 1000;
+ isPublicApiTimeout = true;
+ LOGDEBUG("sendData2 %d ", sendTimeoutSec);
+ }
--- End diff --
Formatting does not conform to [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #105: GEODE-2891 connect-timeout violation in C++ Native ...
Posted by gregt5259 <gi...@git.apache.org>.
Github user gregt5259 commented on the issue:
https://github.com/apache/geode-native/pull/105
Closed by Ernie Burghardt request as duplicated #106
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #105: GEODE-2891 connect-timeout violation in C++ Native ...
Posted by gregt5259 <gi...@git.apache.org>.
Github user gregt5259 commented on the issue:
https://github.com/apache/geode-native/pull/105
I suggest to fix the issue by defining handshake pseudo message within the range probably defined for such pseudo messages by original design i.e.:
typedef enum {
/* Server couldn't read message; handle it like a server side
exception that needs retries */
HANDSHAKE = -3
NOT_PUBLIC_API_WITH_TIMEOUT = -2,
WDYT?
Thanks,
Dr. Gregory Turovets
"…We're all mad here. I'm mad. You're mad."
"How do you know I'm mad?" said Alice.
"You must be," said the Cat, "or you wouldn't have come here."
Alice's Adventures in Wonderland by Lewis Carroll<http://www.livelib.ru/author/157108>.
From: Jacob Barrett [mailto:notifications@github.com]
Sent: Sunday, July 02, 2017 17:46
To: apache/geode-native <ge...@noreply.github.com>
Cc: Gregory Turovets <gr...@amdocs.com>; Author <au...@noreply.github.com>
Subject: Re: [apache/geode-native] GEODE-2891 connect-timeout violation in C++ Native Client (#105)
@pivotal-jbarrett requested changes on this pull request.
Please rebase your changes on develop rather than merge.
Do not include comments with ticket numbers.
Do not include comments with your name or initials.
Do not leave sources commented out, delete or delete not.
Please follow the C++ style of the community.
I am concerned with the approach of trying to define a pseudo message for handshaking to get a different timeout unit. This may bite us in the future when added new messages to the protocol.
There are a few tickets in flight, or soon to be in flight, that address this problem.
https://issues.apache.org/jira/browse/GEODE-3136
https://issues.apache.org/jira/browse/GEODE-3137
I have begun some experiments with GEODE-3136 and should start committing to it in a few days. All API exposed timeouts will be based on std::chrono::duration so you can clearly see what unit of time your time is and the code behind that API doesn't have to guess. GEODE-3137 will address on use cases internally that aren't addressed when updating the public API. Any configuration files that specify timeout will be updated to take a duration string as well in the format of "1234s", "1234ms", etc.
________________________________
In src/cppcache/src/TcrConnection.cpp<https://github.com/apache/geode-native/pull/105#discussion_r125184790>:
> @@ -318,7 +318,9 @@ bool TcrConnection::InitTcrConnection(
LOGFINE("Attempting handshake with endpoint %s for %s%s connection", endpoint,
isClientNotification ? (isSecondary ? "secondary " : "primary ") : "",
isClientNotification ? "subscription" : "client");
- ConnErrType error = sendData(data, msgLengh, connectTimeout, false);
+ // GT GEODE-2891
Do not include comments with your name or the ticket number.
________________________________
In src/cppcache/src/TcrConnection.cpp<https://github.com/apache/geode-native/pull/105#discussion_r125184798>:
> @@ -318,7 +318,9 @@ bool TcrConnection::InitTcrConnection(
LOGFINE("Attempting handshake with endpoint %s for %s%s connection", endpoint,
isClientNotification ? (isSecondary ? "secondary " : "primary ") : "",
isClientNotification ? "subscription" : "client");
- ConnErrType error = sendData(data, msgLengh, connectTimeout, false);
+ // GT GEODE-2891
+ //ConnErrType error = sendData( data, msgLengh, connectTimeout, false );
Do not leave commented out sources, this is what revision control is for.
________________________________
In src/cppcache/src/TcrConnection.cpp<https://github.com/apache/geode-native/pull/105#discussion_r125184917>:
> - // then app has set timeout in millis, change it to microSeconds
- sendTimeoutSec = sendTimeoutSec * 1000;
- isPublicApiTimeout = true;
- LOGDEBUG("sendData2 %d ", sendTimeoutSec);
- } else {
- sendTimeoutSec = sendTimeoutSec * 1000;
- }
+ notPublicApiWithTimeout == TcrMessage::EXECUTE_REGION_FUNCTION_SINGLE_HOP ||
+ // GT GEODE-2891
+ notPublicApiWithTimeout == TcrMessage::HANDSHAKE)
+ {
+ // then app has set timeout in millis, change it to microSeconds
+ sendTimeoutSec = sendTimeoutSec * 1000;
+ isPublicApiTimeout = true;
+ LOGDEBUG("sendData2 %d ", sendTimeoutSec);
+ }
Formatting does not conform to Google C++ Style Guide<https://google.github.io/styleguide/cppguide.html>.
________________________________
In src/cppcache/src/TcrMessage.hpp<https://github.com/apache/geode-native/pull/105#discussion_r125184959>:
> @@ -171,7 +173,8 @@ class CPPCACHE_EXPORT TcrMessage {
GET_DURABLE_CQS_DATA_ERROR = 106,
GET_ALL_WITH_CALLBACK = 107,
PUT_ALL_WITH_CALLBACK = 108,
- REMOVE_ALL = 109
+ REMOVE_ALL = 109,
+ HANDSHAKE = 110
These numbers correspond to protocol message numbers on the server. We can just add one here and expect it not cause issues later.
________________________________
In src/cppcache/src/TcrMessage.hpp<https://github.com/apache/geode-native/pull/105#discussion_r125184965>:
> @@ -44,6 +44,8 @@
#include <map>
#include <vector>
+//
Clean this up.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://github.com/apache/geode-native/pull/105#pullrequestreview-47551893>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AbZcfm-Wvg_K8tzwTd2ACFihvkB1aQLaks5sJ60jgaJpZM4OLjA1>.
This message and the information contained herein is proprietary and confidential and subject to the Amdocs policy statement,
you may review at https://www.amdocs.com/about/email-disclaimer <https://www.amdocs.com/about/email-disclaimer>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #105: GEODE-2891 connect-timeout violation in C++ ...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/105#discussion_r125184790
--- Diff: src/cppcache/src/TcrConnection.cpp ---
@@ -318,7 +318,9 @@ bool TcrConnection::InitTcrConnection(
LOGFINE("Attempting handshake with endpoint %s for %s%s connection", endpoint,
isClientNotification ? (isSecondary ? "secondary " : "primary ") : "",
isClientNotification ? "subscription" : "client");
- ConnErrType error = sendData(data, msgLengh, connectTimeout, false);
+ // GT GEODE-2891
--- End diff --
Do not include comments with your name or the ticket number.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #105: GEODE-2891 connect-timeout violation in C++ ...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/105#discussion_r125184959
--- Diff: src/cppcache/src/TcrMessage.hpp ---
@@ -171,7 +173,8 @@ class CPPCACHE_EXPORT TcrMessage {
GET_DURABLE_CQS_DATA_ERROR = 106,
GET_ALL_WITH_CALLBACK = 107,
PUT_ALL_WITH_CALLBACK = 108,
- REMOVE_ALL = 109
+ REMOVE_ALL = 109,
+ HANDSHAKE = 110
--- End diff --
These numbers correspond to protocol message numbers on the server. We can just add one here and expect it not cause issues later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #105: GEODE-2891 connect-timeout violation in C++ ...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/105#discussion_r125184965
--- Diff: src/cppcache/src/TcrMessage.hpp ---
@@ -44,6 +44,8 @@
#include <map>
#include <vector>
+//
--- End diff --
Clean this up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #105: GEODE-2891 connect-timeout violation in C++ Native ...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on the issue:
https://github.com/apache/geode-native/pull/105
@gregt5259 please close this pull request since you opened #106.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #105: GEODE-2891 connect-timeout violation in C++ Native ...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on the issue:
https://github.com/apache/geode-native/pull/105
> I suggest to fix the issue by defining handshake pseudo message within the range probably defined for such pseudo messages by original design i.e.:
> typedef enum {
> /* Server couldn't read message; handle it like a server side
> exception that needs retries */
> HANDSHAKE = -3
> NOT_PUBLIC_API_WITH_TIMEOUT = -2,
I think that would certainly work better but I think ultimately the fix is to remove the ambiguity between the C++ API and the Server API by strongly typing the duration values as addressed in the mentioned tickets. All duration values can be cast to the internal API supported unit without any ambiguity. After which there is no need to create this pseudo message to change the unit of the ambiguous int value.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #105: GEODE-2891 connect-timeout violation in C++ ...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/105#discussion_r125184798
--- Diff: src/cppcache/src/TcrConnection.cpp ---
@@ -318,7 +318,9 @@ bool TcrConnection::InitTcrConnection(
LOGFINE("Attempting handshake with endpoint %s for %s%s connection", endpoint,
isClientNotification ? (isSecondary ? "secondary " : "primary ") : "",
isClientNotification ? "subscription" : "client");
- ConnErrType error = sendData(data, msgLengh, connectTimeout, false);
+ // GT GEODE-2891
+ //ConnErrType error = sendData( data, msgLengh, connectTimeout, false );
--- End diff --
Do not leave commented out sources, this is what revision control is for.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---