[GitHub] geode-native issue #106: GEODE-2891 connect-timeout violation in C++ Native ...
Github user gregt5259 commented on the issue: https://github.com/apache/geode-native/pull/106 Pull request is closed after passing of all checks and marked as resolved in Jira GEODE-2891 with resolution "Won't fix" as following: GEODE-2891 - connect-timeout violation in C++ Native Client is superceded by GEODE-3137 - Replace all time values internally with std::chrono types --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode-native pull request #106: GEODE-2891 connect-timeout violation in C++ ...
Github user gregt5259 closed the pull request at: https://github.com/apache/geode-native/pull/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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode-native issue #105: GEODE-2891 connect-timeout violation in C++ Native ...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode-native pull request #105: GEODE-2891 connect-timeout violation in C++ ...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode-native issue #106: GEODE-2891 connect-timeout violation in C++ Native ...
Github user gregt5259 commented on the issue: https://github.com/apache/geode-native/pull/106 In my mind we talk about the some kind of redesign rather than about correct change: the original design is based de-facto exactly on using âmagic numbersâ; configurable measurements units in an explicit form are missing in the original design. Yes, my solution solves the problem in the code implementing accordingly to the original design and quite could be realized in the code implementing original design without waiting for future redesigns and refactoring, especially with undefined due date. 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:notificati...@github.com] Sent: Thursday, July 06, 2017 17:35 To: apache/geode-native Cc: Gregory Turovets ; Mention Subject: Re: [apache/geode-native] GEODE-2891 connect-timeout violation in C++ Native Client (#106) @gregt5259<https://github.com/gregt5259> This is a solution to the problem but not the solution we as committers are comfortable committing as it directly conflicts with the correct change, which is to use type safe durations rather than magic number math and system wide properties to create a confusing array of time values. If you want this change sooner than later you could implement it using std::chrono::duration as outlined in GEODE-3137 or maintain a fork with your change in it. â You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://github.com/apache/geode-native/pull/106#issuecomment-313414625>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AbZcfv8z-a_Qfl4VwbQmqK8WI7aM9k9aks5sLPB0gaJpZM4OOTnp>. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode-native issue #106: GEODE-2891 connect-timeout violation in C++ Native ...
Github user gregt5259 commented on the issue: https://github.com/apache/geode-native/pull/106 Did I understood correct that there are no issues found during the code review in the reviewed code? Probably the decision regarding the accepting of the pull request doesnât depend in this case on the code quality but should depend on ETA for GEODE-3136<https://issues.apache.org/jira/browse/GEODE-3136> and GEODE-3137<https://issues.apache.org/jira/browse/GEODE-3136>, on the appropriate next client version deployment readiness et cetera. If these dates will be published, that will assist us within the company to take decision whether we may wait for this client version or will require to accept the pull request even as the temporary fix. Thanks, Dr. Gregory Turovets From: Jacob Barrett [mailto:notificati...@github.com] Sent: Wednesday, July 05, 2017 17:41 To: apache/geode-native Cc: Gregory Turovets ; Author Subject: Re: [apache/geode-native] GEODE-2891 connect-timeout violation in C++ Native Client (#106) @pivotal-jbarrett requested changes on this pull request. I am not in favor of accepting this pull request on the heals of correcting all timeouts via GEODE-3136<https://issues.apache.org/jira/browse/GEODE-3136> and GEODE-3137<https://issues.apache.org/jira/browse/GEODE-3136> as mentioned in pull #105<https://github.com/apache/geode-native/pull/105>. â 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/106#pullrequestreview-48073734>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AbZcfvST1gIEk8aYilBGwKHkEhPC0_ecks5sK6CXgaJpZM4OOTnp>. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode-native pull request #106: GEODE-2891 connect-timeout violation in C++ ...
GitHub user gregt5259 opened a pull request: https://github.com/apache/geode-native/pull/106 GEODE-2891 connect-timeout violation in C++ Native Client Pseudo message ID is added to recognize handshake message and use the correct time unit depending on the appropriate configuration parameter. 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/106.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 #106 commit 28b6536d85990aef41575d44ef11768dbefdb4f0 Author: gregt5259 Date: 2017-07-05T10:28:53Z GEODE-2891 connect-timeout violation in C++ Native Client: fix Pseudo message ID is added to recognize handshake message and use the correct time unit depending on the appropriate configuration parameter. commit be95892927b35e66d5c507f136e394aa4b1b0730 Author: gregt5259 Date: 2017-07-05T10:35:28Z Remove the file added by mistake commit 36b5cbb615fca0d337e7902f2d74f6f994d09585 Author: gregt5259 Date: 2017-07-05T11:13:54Z Revert "Remove the file added by mistake" This reverts commit be95892927b35e66d5c507f136e394aa4b1b0730. commit ad7113401fbe2c2f35ec52d96421dd396d8b6b01 Author: gregt5259 Date: 2017-07-05T11:14:07Z Revert "GEODE-2891 connect-timeout violation in C++ Native Client: fix" This reverts commit 28b6536d85990aef41575d44ef11768dbefdb4f0. commit f24ad29356488f9b8561a6af144f582a83367c96 Author: gregt5259 Date: 2017-07-05T11:23:54Z GEODE-2891 connect-timeout violation in C++ Native Client: fix Pseudo message ID is added to recognize handshake message and use the correct time unit depending on the appropriate configuration parameter. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode-native issue #105: GEODE-2891 connect-timeout violation in C++ Native ...
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:notificati...@github.com]
Sent: Sunday, July 02, 2017 17:46
To: apache/geode-native
Cc: Gregory Turovets ; Author
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("
[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 Date: 2017-07-02T07:27:50Z Merge remote-tracking branch 'refs/remotes/apache/develop' into develop commit c5743fea51b390338f9cd548b5aef3f888c6ce0a Author: gregt5259 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
