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/05 13:05:19 UTC
[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 <gr...@amdocs.com>
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 <gr...@amdocs.com>
Date: 2017-07-05T10:35:28Z
Remove the file added by mistake
commit 36b5cbb615fca0d337e7902f2d74f6f994d09585
Author: gregt5259 <gr...@amdocs.com>
Date: 2017-07-05T11:13:54Z
Revert "Remove the file added by mistake"
This reverts commit be95892927b35e66d5c507f136e394aa4b1b0730.
commit ad7113401fbe2c2f35ec52d96421dd396d8b6b01
Author: gregt5259 <gr...@amdocs.com>
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 <gr...@amdocs.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #106: 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/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:notifications@github.com]
Sent: Wednesday, July 05, 2017 17:41
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 (#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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #106: 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/106
@echobravopapa the branching scheme only applies to committers working directly with the Geode repositories. Non-committers use pull requests which render the branching/girflow requirement null.
---
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 #106: 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/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:notifications@github.com]
Sent: Thursday, July 06, 2017 17:35
To: apache/geode-native <ge...@noreply.github.com>
Cc: Gregory Turovets <gr...@amdocs.com>; Mention <me...@noreply.github.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #106: GEODE-2891 connect-timeout violation in C++ Native ...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on the issue:
https://github.com/apache/geode-native/pull/106
@pivotal-jbarrett fair enough on the branching point, I think the rest is still a valid ask
---
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 #106: 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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #106: GEODE-2891 connect-timeout violation in C++ Native ...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on the issue:
https://github.com/apache/geode-native/pull/106
@gregt5259 a couple quick items of feedback that need to be addressed before this is reviewed:
- please squash your commits, this cleans up the diffs for everyone to better review your contribution
please close [#105](https://github.com/apache/geode-native/pull/105) as duplicate of this PR
- please have a look at the [Code Contribution](https://cwiki.apache.org/confluence/display/GEODE/Code+contributions) section of the WIKI
-- for one you will see that we are using GitFlow and your PR should be coming in from a feature branch (e.g. feature/GEODE-2891) instead of your develop branch
Thanks!
---
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 #106: 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/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 #106: 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/106
@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.
---
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.
---