You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by revans2 <gi...@git.apache.org> on 2015/04/06 19:15:34 UTC
[GitHub] storm pull request: STORM-750: Set Config serialVersionUID
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/507
STORM-750: Set Config serialVersionUID
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/incubator-storm STORM-750
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/507.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 #507
----
commit 3a5d3a7892086f4a99f0b35f16a5207eb0522d5d
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2015-04-06T17:13:35Z
STORM-750: Set Config Serialization
----
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90165302
Did this just go from pull request to merged in less than 15 minutes?
I know it's a very minor change, and has all the requisite +1s, but that seems a little hasty to me.
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/507
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90160812
+1
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90210685
@d2r @ptgoetz
Crud you are right, I thought that was dropped with the move to retroactive -1. Sorry about that. If anyone wants me to revert the change and wait until tomorrow I will be happy to.
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90296417
@knusbaum I agree.
Seems like an AND vs. OR issue between those two sentences. IMO it should be AND.
To me, it seems only fair to allow the sun to pass through all time zones before committing a patch given the geographical distribution of out community.
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90208538
@revans2 Yeah, the bylaws do say a minimum of 1 day for code changes.
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90160883
+1
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90290248
We should update the wording of the bylaws, then. I read through them when this went in and (apparently) misinterpreted them.
The rules for code changes state both: "The code can be committed after the first +1" and "Minimum Length: 1 day from initial patch"
I thought, like @revans2, that we'd dropped the 1-day waiting for the retroactive -1. I'm fine with either, but whichever we choose, we should make it more clear in the wording of the bylaws.
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90160981
+1.
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90211059
@revans2,
True, we can retroactively revert if a -1 comes in, but the bylaws state that the minimum approval time for a code change is 1 day.
That's there to give everyone a chance to review (we're all in different time zones). In this case it's a very minor change and I don't think anyone will -1 it, but we should at least give everyone the chance to review a code change.
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90218104
@revans2 I thought about that (-1 to force a revert), but I decided against it. It was an honest mistake made in the spirit of adhering to the bylaws. As I mentioned earlier it is such an innocuous change that I doubt anyone would be against it.
Nonetheless, in the future I think we should stick with the 24-hour guideline.
I'm +1 for leaving it as is, if anyone disagrees, we can always revert.
---
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] storm pull request: STORM-750: Set Config serialVersionUID
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/507#issuecomment-90205871
@ptgoetz,
I agree that it was rather fast (7 mins), but I thought with the bylaws allowing for a retroactive -1 to revert the change, that it was not that big of a deal.
---
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.
---