Don't we have some sort of policy about an individual merging commits that he/she uploaded? Because these three changes: https://gerrit.wikimedia.org/r/36801
https://gerrit.wikimedia.org/r/36812
https://gerrit.wikimedia.org/r/36813
Were all uploaded and submitted in a matter of minutes by the same person, and each is a fix for errors in the commit before it. It kind of defeats the point of having code review in the first place. *--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com
| tylerromeo@gmail.com
On Tue, Dec 4, 2012 at 3:24 PM, Tyler Romeo tylerromeo@gmail.com wrote:
Don't we have some sort of policy about an individual merging commits that he/she uploaded?
Yes. We've been over this a dozen times--if you're on a repository that has multiple maintainers (ie: you're not the only one, so you're always self-merging), you should almost never merge your own code unless you're fixing an immediate problem (site outage, sytax errors).
Merging non-critical code so fast before anyone even has a chance to say "hang on a minute" is especially bad.
-Chad
On Tue, Dec 4, 2012 at 3:27 PM, Chad innocentkiller@gmail.com wrote:
On Tue, Dec 4, 2012 at 3:24 PM, Tyler Romeo tylerromeo@gmail.com wrote:
Don't we have some sort of policy about an individual merging commits that he/she uploaded?
Yes. We've been over this a dozen times--if you're on a repository that has multiple maintainers (ie: you're not the only one, so you're always self-merging), you should almost never merge your own code unless you're fixing an immediate problem (site outage, sytax errors).
In fact, I'm tired of repeating this problem, so I started a change to actually enforce this policy[0]. We'll probably need to tweak it further to allow for the exceptions we actually want. Review welcome.
-Chad
On Tue, 04 Dec 2012 12:37:02 -0800, Chad innocentkiller@gmail.com wrote:
On Tue, Dec 4, 2012 at 3:27 PM, Chad innocentkiller@gmail.com wrote:
On Tue, Dec 4, 2012 at 3:24 PM, Tyler Romeo tylerromeo@gmail.com wrote:
Don't we have some sort of policy about an individual merging commits that he/she uploaded?
Yes. We've been over this a dozen times--if you're on a repository that has multiple maintainers (ie: you're not the only one, so you're always self-merging), you should almost never merge your own code unless you're fixing an immediate problem (site outage, sytax errors).
In fact, I'm tired of repeating this problem, so I started a change to actually enforce this policy[0]. We'll probably need to tweak it further to allow for the exceptions we actually want. Review welcome.
-Chad
Doesn't TWN's bot self-review? Might need to add an exception for that before merging.
On Tue, Dec 4, 2012 at 3:46 PM, Daniel Friesen daniel@nadir-seen-fire.com wrote:
On Tue, 04 Dec 2012 12:37:02 -0800, Chad innocentkiller@gmail.com wrote:
On Tue, Dec 4, 2012 at 3:27 PM, Chad innocentkiller@gmail.com wrote:
On Tue, Dec 4, 2012 at 3:24 PM, Tyler Romeo tylerromeo@gmail.com wrote:
Don't we have some sort of policy about an individual merging commits that he/she uploaded?
Yes. We've been over this a dozen times--if you're on a repository that has multiple maintainers (ie: you're not the only one, so you're always self-merging), you should almost never merge your own code unless you're fixing an immediate problem (site outage, sytax errors).
In fact, I'm tired of repeating this problem, so I started a change to actually enforce this policy[0]. We'll probably need to tweak it further to allow for the exceptions we actually want. Review welcome.
-Chad
Doesn't TWN's bot self-review? Might need to add an exception for that before merging.
And this is why I don't like to self merge--I had totally forgotten that ;-)
-Chad
On Dec 4, 2012, at 9:46 PM, Daniel Friesen daniel@nadir-seen-fire.com wrote:
On Tue, 04 Dec 2012 12:37:02 -0800, Chad innocentkiller@gmail.com wrote:
On Tue, Dec 4, 2012 at 3:27 PM, Chad innocentkiller@gmail.com wrote:
On Tue, Dec 4, 2012 at 3:24 PM, Tyler Romeo tylerromeo@gmail.com wrote:
Don't we have some sort of policy about an individual merging commits that he/she uploaded?
Yes. We've been over this a dozen times--if you're on a repository that has multiple maintainers (ie: you're not the only one, so you're always self-merging), you should almost never merge your own code unless you're fixing an immediate problem (site outage, sytax errors).
In fact, I'm tired of repeating this problem, so I started a change to actually enforce this policy[0]. We'll probably need to tweak it further to allow for the exceptions we actually want. Review welcome.
-Chad
Doesn't TWN's bot self-review? Might need to add an exception for that before merging.
I'm not sure in which part of the flow rules.pl is applied but maybe it can be enforced the other way around?
Instead of restricting Submit, restrict CR scores. Submission in turn only has to be restricted to CR+2.
But yeah, we need to either whitelist L10n-bot from this restriction or make those commits auto-merge in a different way.
-- Krinkle
On Wed, Dec 5, 2012 at 5:05 AM, Krinkle krinklemail@gmail.com wrote:
On Dec 4, 2012, at 9:46 PM, Daniel Friesen daniel@nadir-seen-fire.com wrote:
On Tue, 04 Dec 2012 12:37:02 -0800, Chad innocentkiller@gmail.com wrote:
On Tue, Dec 4, 2012 at 3:27 PM, Chad innocentkiller@gmail.com wrote:
On Tue, Dec 4, 2012 at 3:24 PM, Tyler Romeo tylerromeo@gmail.com wrote:
Don't we have some sort of policy about an individual merging commits that he/she uploaded?
Yes. We've been over this a dozen times--if you're on a repository that has multiple maintainers (ie: you're not the only one, so you're always self-merging), you should almost never merge your own code unless you're fixing an immediate problem (site outage, sytax errors).
In fact, I'm tired of repeating this problem, so I started a change to actually enforce this policy[0]. We'll probably need to tweak it further to allow for the exceptions we actually want. Review welcome.
-Chad
Doesn't TWN's bot self-review? Might need to add an exception for that before merging.
I'm not sure in which part of the flow rules.pl is applied but maybe it can be enforced the other way around?
Instead of restricting Submit, restrict CR scores. Submission in turn only has to be restricted to CR+2.
But yeah, we need to either whitelist L10n-bot from this restriction or make those commits auto-merge in a different way.
And behold, there are docs:
https://gerrit-review.googlesource.com/Documentation/prolog-cookbook.html
https://gerrit-review.googlesource.com/Documentation/prolog-change-facts.htm...
-Chad
Hi Guys, How do I get a specific version of an extension using git? I want to get Validator 0.4.1.4 and Maps 1.0.5, but I can't figure out how to use git to do this... Thanks, Aran
On 05.12.2012 14:39, Aran Dunkley wrote:
Hi Guys, How do I get a specific version of an extension using git? I want to get Validator 0.4.1.4 and Maps 1.0.5, but I can't figure out how to use git to do this...
git always clones the entire repository, including all version. So, you clone, and then use git checkout to get whatever branch or tag you want.
-- daniel
Le 04/12/12 21:24, Tyler Romeo a écrit :
Don't we have some sort of policy about an individual merging commits that he/she uploaded? Because these three changes: https://gerrit.wikimedia.org/r/36801
https://gerrit.wikimedia.org/r/36812
https://gerrit.wikimedia.org/r/36813
Were all uploaded and submitted in a matter of minutes by the same person, and each is a fix for errors in the commit before it. It kind of defeats the point of having code review in the first place.
Overall there is a lot of issues in this class such as :
- shelling out to possible non existent command (slooow) - using file locks (everytime you want a new UID it writes a file, lock it, generate the uid, unlock the file and delete it. That seems slow to me and probably not going to scale. - There is ton of code duplication when the number of bits we want should be a parameter to a generic function.
uniqid( ) will probably give you what you want without having to shell out. It is based on microtime() but you could add more entropy by passing a string prefix as first arg and true as second arg to add in pseudo random value.
Anyway that looks like a work in progress, I have submitted a change to revert the commits from master:
https://gerrit.wikimedia.org/r/36961
Some notes (copied from private email): * It only creates the lock file the first time. * The functions with different bits are not just the same thing with more bits. Trying to abstract more just made it more confusing. * The point is to also have something with better properties than uniqid. Also I ran large for loops calling those functions and timed it on my laptop back when I was working on that and found it reasonable (if you needed to insert faster you'd probably have DB overload anyway). * hostid seems pretty common and is on the random wmf servers I tested a while back. If there is some optimization there for third parties that don't have it, of course it would be welcomed. ---- At any rate, I changed the revert summary though Timo beat me to actually merging the revert. My main issue is the authorship breakage and the fact that the "split of" change wasn't +2'd by a different person. I was also later asked to add tests (36816), which should ideally would have been required in the first patch rather than as a second one; not a big deal but it's a plus to consolidating the changes after a revert.
That said, the change was actually a class split off verbatim from https://gerrit.wikimedia.org/r/#/c/16696/ (which was pending for ages), so it's not like the change was in gerrit for a split-second and then merged. I think the process should have been better here though it's not a huge deal as it may seem at first glance.
-- View this message in context: http://wikimedia.7.n6.nabble.com/Really-Fast-Merges-tp4990838p4990911.html Sent from the Wikipedia Developers mailing list archive at Nabble.com.
There were 132 days for anybody to review and comment on the technical approach in the UID class.
— Patrick
On Wed, Dec 5, 2012 at 10:09 AM, Aaron Schulz aschulz4587@gmail.com wrote:
Some notes (copied from private email):
- It only creates the lock file the first time.
- The functions with different bits are not just the same thing with more
bits. Trying to abstract more just made it more confusing.
- The point is to also have something with better properties than uniqid.
Also I ran large for loops calling those functions and timed it on my laptop back when I was working on that and found it reasonable (if you needed to insert faster you'd probably have DB overload anyway).
- hostid seems pretty common and is on the random wmf servers I tested a
while back. If there is some optimization there for third parties that don't have it, of course it would be welcomed.
At any rate, I changed the revert summary though Timo beat me to actually merging the revert. My main issue is the authorship breakage and the fact that the "split of" change wasn't +2'd by a different person. I was also later asked to add tests (36816), which should ideally would have been required in the first patch rather than as a second one; not a big deal but it's a plus to consolidating the changes after a revert.
That said, the change was actually a class split off verbatim from https://gerrit.wikimedia.org/r/#/c/16696/ (which was pending for ages), so it's not like the change was in gerrit for a split-second and then merged. I think the process should have been better here though it's not a huge deal as it may seem at first glance.
-- View this message in context: http://wikimedia.7.n6.nabble.com/Really-Fast-Merges-tp4990838p4990911.html Sent from the Wikipedia Developers mailing list archive at Nabble.com.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
132 days? It was uploaded onto Gerrit just recently. Many of the people here (including myself) only get notice of changes if it's discussed on the mailing list or if a change is uploaded to Gerrit.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Dec 5, 2012 at 1:13 PM, Patrick Reilly preilly@wikimedia.orgwrote:
There were 132 days for anybody to review and comment on the technical approach in the UID class.
— Patrick
On Wed, Dec 5, 2012 at 10:09 AM, Aaron Schulz aschulz4587@gmail.com wrote:
Some notes (copied from private email):
- It only creates the lock file the first time.
- The functions with different bits are not just the same thing with more
bits. Trying to abstract more just made it more confusing.
- The point is to also have something with better properties than uniqid.
Also I ran large for loops calling those functions and timed it on my
laptop
back when I was working on that and found it reasonable (if you needed to insert faster you'd probably have DB overload anyway).
- hostid seems pretty common and is on the random wmf servers I tested a
while back. If there is some optimization there for third parties that
don't
have it, of course it would be welcomed.
At any rate, I changed the revert summary though Timo beat me to actually merging the revert. My main issue is the authorship breakage and the fact that the "split of" change wasn't +2'd by a different person. I was also later asked to add tests (36816), which should ideally would have been required in the first patch rather than as a second one; not a big deal
but
it's a plus to consolidating the changes after a revert.
That said, the change was actually a class split off verbatim from https://gerrit.wikimedia.org/r/#/c/16696/ (which was pending for ages),
so
it's not like the change was in gerrit for a split-second and then
merged. I
think the process should have been better here though it's not a huge
deal
as it may seem at first glance.
-- View this message in context:
http://wikimedia.7.n6.nabble.com/Really-Fast-Merges-tp4990838p4990911.html
Sent from the Wikipedia Developers mailing list archive at Nabble.com.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Tyler,
It was uploaded originally in the following commit: https://gerrit.wikimedia.org/r/#/c/16696/ dated Jul 25, 2012 4:11 PM by Aaron Schulz.
The only thing that I did was to break it off into a separate commit: https://gerrit.wikimedia.org/r/#/c/36801/
So, the point that I was attempting to make was that it in unaltered form was available for review for; 132 days or 4 months, 9 days.
The mistake that I made was that I didn't use Forge Author and Forge Committer access control rights in Gerrit. As, well as NOT adding it to the auto loader initially.
— Patrick
On Wed, Dec 5, 2012 at 10:21 AM, Tyler Romeo tylerromeo@gmail.com wrote:
132 days? It was uploaded onto Gerrit just recently. Many of the people here (including myself) only get notice of changes if it's discussed on the mailing list or if a change is uploaded to Gerrit.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Dec 5, 2012 at 1:13 PM, Patrick Reilly preilly@wikimedia.orgwrote:
There were 132 days for anybody to review and comment on the technical approach in the UID class.
— Patrick
On Wed, Dec 5, 2012 at 10:09 AM, Aaron Schulz aschulz4587@gmail.com wrote:
Some notes (copied from private email):
- It only creates the lock file the first time.
- The functions with different bits are not just the same thing with more
bits. Trying to abstract more just made it more confusing.
- The point is to also have something with better properties than uniqid.
Also I ran large for loops calling those functions and timed it on my
laptop
back when I was working on that and found it reasonable (if you needed to insert faster you'd probably have DB overload anyway).
- hostid seems pretty common and is on the random wmf servers I tested a
while back. If there is some optimization there for third parties that
don't
have it, of course it would be welcomed.
At any rate, I changed the revert summary though Timo beat me to actually merging the revert. My main issue is the authorship breakage and the fact that the "split of" change wasn't +2'd by a different person. I was also later asked to add tests (36816), which should ideally would have been required in the first patch rather than as a second one; not a big deal
but
it's a plus to consolidating the changes after a revert.
That said, the change was actually a class split off verbatim from https://gerrit.wikimedia.org/r/#/c/16696/ (which was pending for ages),
so
it's not like the change was in gerrit for a split-second and then
merged. I
think the process should have been better here though it's not a huge
deal
as it may seem at first glance.
-- View this message in context:
http://wikimedia.7.n6.nabble.com/Really-Fast-Merges-tp4990838p4990911.html
Sent from the Wikipedia Developers mailing list archive at Nabble.com.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Ah OK. That's my fault, then. I must have missed the initial upload of the change. By the way, what exactly is the purpose of the RDBStore and UIDGenerator classes? It looks interesting, but I'm just wondering what the core or extensions will use it for.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Dec 5, 2012 at 1:29 PM, Patrick Reilly preilly@wikimedia.orgwrote:
Tyler,
It was uploaded originally in the following commit: https://gerrit.wikimedia.org/r/#/c/16696/ dated Jul 25, 2012 4:11 PM by Aaron Schulz.
The only thing that I did was to break it off into a separate commit: https://gerrit.wikimedia.org/r/#/c/36801/
So, the point that I was attempting to make was that it in unaltered form was available for review for; 132 days or 4 months, 9 days.
The mistake that I made was that I didn't use Forge Author and Forge Committer access control rights in Gerrit. As, well as NOT adding it to the auto loader initially.
— Patrick
On Wed, Dec 5, 2012 at 10:21 AM, Tyler Romeo tylerromeo@gmail.com wrote:
132 days? It was uploaded onto Gerrit just recently. Many of the people here (including myself) only get notice of changes if it's discussed on
the
mailing list or if a change is uploaded to Gerrit.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Dec 5, 2012 at 1:13 PM, Patrick Reilly <preilly@wikimedia.org wrote:
There were 132 days for anybody to review and comment on the technical approach in the UID class.
— Patrick
On Wed, Dec 5, 2012 at 10:09 AM, Aaron Schulz aschulz4587@gmail.com wrote:
Some notes (copied from private email):
- It only creates the lock file the first time.
- The functions with different bits are not just the same thing with
more
bits. Trying to abstract more just made it more confusing.
- The point is to also have something with better properties than
uniqid.
Also I ran large for loops calling those functions and timed it on my
laptop
back when I was working on that and found it reasonable (if you
needed to
insert faster you'd probably have DB overload anyway).
- hostid seems pretty common and is on the random wmf servers I
tested a
while back. If there is some optimization there for third parties that
don't
have it, of course it would be welcomed.
At any rate, I changed the revert summary though Timo beat me to
actually
merging the revert. My main issue is the authorship breakage and the
fact
that the "split of" change wasn't +2'd by a different person. I was
also
later asked to add tests (36816), which should ideally would have been required in the first patch rather than as a second one; not a big
deal
but
it's a plus to consolidating the changes after a revert.
That said, the change was actually a class split off verbatim from https://gerrit.wikimedia.org/r/#/c/16696/ (which was pending for
ages),
so
it's not like the change was in gerrit for a split-second and then
merged. I
think the process should have been better here though it's not a huge
deal
as it may seem at first glance.
-- View this message in context:
http://wikimedia.7.n6.nabble.com/Really-Fast-Merges-tp4990838p4990911.html
Sent from the Wikipedia Developers mailing list archive at Nabble.com.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
RDBStore is shelfed as a reference for now. The idea was to partition sql table across multiple DB servers using a consistent hash of some column. There no longer would be the convenience of autoincrement columns so UIDs are a way to make unique ids without a central table or counter.
In some cases, like when the primary key is the uid column, duplicate detection can be enforce by the DB since duplicate values would map to the same partition table and that table would have a unique index, causing a duplicate key error. This could allow for slightly smaller uids to be used with the comfort of knowing that in the unlikely event of a rare collision, it will be detected. This is why it had several uid functions. It might be nice to add a standard UUID1 and UUID4 function, though they were not useful for RDB store for B-TREE reasons.
-- View this message in context: http://wikimedia.7.n6.nabble.com/Really-Fast-Merges-tp4990838p4990931.html Sent from the Wikipedia Developers mailing list archive at Nabble.com.
On 05/12/12 21:01, Aaron Schulz wrote:
RDBStore is shelfed as a reference for now. The idea was to partition sql table across multiple DB servers using a consistent hash of some column. There no longer would be the convenience of autoincrement columns so UIDs are a way to make unique ids without a central table or counter.
Why do you need a UID? The autoincrement id we use in most tables can (should) serve as UID.
It needs a little care when sending the inserts, but it's straighforward. It can easily be done by a layer on top of our db classes (transparent to the application).
What table were you planning to partition (and how) that you can't use the id for the partitioning?
On Thu, Dec 6, 2012 at 9:45 AM, Platonides Platonides@gmail.com wrote:
Why do you need a UID? The autoincrement id we use in most tables can (should) serve as UID.
It needs a little care when sending the inserts, but it's straighforward. It can easily be done by a layer on top of our db classes (transparent to the application).
What table were you planning to partition (and how) that you can't use the id for the partitioning?
Can we please spilt further discussions about this off to a new topic? It has very little to do with the original discussion of this list topic (the merging of your own code in our vcs'es)
I share some blame for the existence of this thread. I spotted the git author issue after that commit was merged and was too lazy to revert and fix it. I personally tend to dislike reverting stuff way more than I should (like a prior schema change that was merged without the site being updated). I should have just reverted that immediately and left a new patch waiting for +2.
Patches by person A that just split out a class or function made by person B should still be looked at by someone other than person A. I think it's a border case, but leaning on the side of caution is the best bet. It sucks to have the code break due to something that was accidentally not copied. I don't think it's worth reverting something like that just for being self-merged (which is why didn't), but it's good practice to avoid. If a string of basic follow-ups are needed and people complain, it might be worth reverting though (like what happened here). We can always add patches back into master after giving it a second look, so reverting isn't always a huge deal and need not be stigmatizing. I need to get used to the revert button more; lesson learned. :)
-- View this message in context: http://wikimedia.7.n6.nabble.com/Really-Fast-Merges-tp4990838p4990923.html Sent from the Wikipedia Developers mailing list archive at Nabble.com.
On Dec 5, 2012, at 7:13 PM, Patrick Reilly preilly@wikimedia.org wrote:
There were 132 days for anybody to review and comment on the technical approach in the UID class.
— Patrick
Even if all people involved had seen in a 100 times, self-merging is a social rule separate from that. That was the reason it was brought up, and the reason it was subsequently reverted again. Nothing personal and not (directly) related to the contents of the commit itself.
-- Krinkle
wikitech-l@lists.wikimedia.org