There's a tool called Gerrit, it's uses git, reviewers can view two code versions sided by side from a web page, it also allows reviewer comments.
Is anyone familiar with these code review tools? The issue doesn't appear to be the code; it appears that the current development framework doesn't allow review to be documented. You can't expect to see only minor changes in actively developed software after 3 years. That's the other issue, the long release cycle, the best way to fix that is to volunteer some assistance. I haven't worked to deadlines, I've spent a lot of time carefully reviewing my own code because of the lack of peer review, I hadn't expected that to appear as thrashing for solutions, rather I viewed it as refinement or refactoring. What sort of time frame are you suggesting for a 2.2.1 branch for changes to be incorporated in a measured fashion? What resources are needed to perform this work? How many releases are you proposing in this time frame? Keep in mind there are a lot of very important race conditon / visibility fixes that need to make it to release sooner rather than later. Peter. ----- Original message ----- > > Hi Jeff: > > Interesting point. I don't think the foundation is against Git. There > are now incubator podlings using it, and infra@ is beginning to support > it (http://git-wip-us.apache.org). I think the issue, at least for > River, is just the friction of switching. There are projects switching > over (Apache Flex, for instance), and as more of them switch, their > experiences will be helpful. > > It's something to look into. Perhaps as we split off modules from the > core, there's possibility of changes. > > My original point, however, stands. The way we've been doing things, > saying for sure what's in the trunk is an exercise in forensic > analysis. Which is a problem because there are real commercial users of > the codebase, and we'd like to have more of them. When a code librarian > or architectural review board at an enterprise evaluates whether > "jsk-platform.jar" version 2.3.0 goes into the list of approved jars, > they need to know precisely what the changes are. > > Part of our problem is that we've gone so long without a release. That > should not happen again. > > But the other thing is that the "commit-then-review" model assumed that > changes to existing code would be minor in nature, and supported by the > test suite. In that scenario, review is then trivial. What we've had > is broad-ranging changes to both the code (including critical subsystems > like the DynamicPolicyProvider) and the test suites, plus API changes to > the Starter package, plus claims of huge throughput increases, all of > which makes review somewhat more challenging. As I said before, it's > not that I question the outcome. It's that I have trouble thouroughly > evaluating that outcome on this kind of scale. > > So, the two-pronged approach I'm trying to promote here is to (1) come > up with a plan to vet code additions and changes that will eventually go > into a release stream after 2.2.1, then (2) see what we can do with the > process so that we don't need to repeat (1). > > Cheers, > > Greg. > > > On Sat, 2013-04-27 at 14:29, Jeff Ramsdale wrote: > > I disagree that scm choice is orthogonal to workflow and policy because > > some scms and related tooling support a given workflow much better than > > others. It's the combination of a given scm and workflow that should be > > evaluated versus others. I have a great deal of respect for the Apache > > Foundation and its history, but this battle has already been fought and won > > on GitHub. It is beginning to be a professional liability not to have git > > experience, and with good reason. > > > > Several years ago I had some epic debates on this topic with a Ruby hacker > > friend of mine. I was on the svn side. At best I held my own in the > > arguments, but now I use git exclusively and wouldn't go back. And by that > > I mean I would seriously question taking a job at a shop that didn't use > > git (or Mercurial) because of what it says about their resistance to > > change. I know I'm not the only one that thinks this way as I've seen > > plenty of evidence the Apache Foundation's reputation is suffering as a > > result of its position with respect to git and the corresponding > > pull-request-based development model. > > > > -j > > > > > > On Sat, Apr 27, 2013 at 12:30 AM, Greg Trasuk <[email protected]>wrote: > > > > > > > > I think I might have been the one shooting you down ;-). To be clear, > > > we can think about git (although there still seems to be some skepticism > > > within Apache), but speaking (as always) just for myself, I think the > > > question of scm choice is orthogonal to workflow and policy. We should > > > work out workflow and policy first, then choose tools. I also get > > > nervous about big changes. > > > > > > Cheers, > > > > > > Greg. > > > > > > On Sat, 2013-04-27 at 00:51, Jeff Ramsdale wrote: > > > > I sorta got shot down a little while ago for suggesting git be > > > considered, > > > > but that's exactly the way branches and pull requests in git are > > > handled. I > > > > understand it can be done with other scms, it's just easier with git. > > > > Non-bindingly, I agree with you. > > > > > > > > -j > > > > > > > > > > > > On Fri, Apr 26, 2013 at 6:41 PM, Greg Trasuk <[email protected]> > > > wrote: > > > > > > > > > > > > > > > > > > > Hi all: > > > > > > > > > > Peter brings up an excellent point here, something that I've found > > > > > troubling in this release process. It is exceedingly difficult to > > > > > identify what changes have been made to the code, and why, or to trace > > > > > changes to JIRA issues. By extension it's very hard to identify which > > > > > revisions have been or should be applied to a branch, and what bugs > > > have > > > > > been fixed in a branch. > > > > > > > > > > Just throwing this out for discussion - I'm coming around to the view > > > > > that we should adopt a policy of review-then-commit for changes to the > > > > > trunk and any release branches. > > > > > > > > > > I'm thinking that when someone does a bug fix or some work on a > > > feature, > > > > > we should: > > > > > > > > > > - do that work on a branch (call this the 'working' branch for this > > > > > discussion) > > > > > - package that work as either a patch or a list of revisions to merge > > > > > - put that package into a JIRA issue (which may already exist if we're > > > > > doing a bug fix). > > > > > - identify which branches the patch should be applied to > > > > > - review, discuss, and vote on the patch in the JIRA issue > > > > > - then apply the patch to those branches, referencing the JIRA issue > > > > > in > > > > > the commit messages > > > > > - ideally, terminate the working branch. Future work proceeds on a > > > > > new > > > > > branch from the trunk. > > > > > > > > > > That way, it would be easy to trace changes in the trunk and release > > > > > branches, and hence to generate an accurate release notice. > > > > > > > > > > Also, I'd suggest that the "FIX_VERSION" field in JIRA belongs to the > > > > > release manager for a given release - he or she will update the field > > > in > > > > > the requisite issues as part of the release process. > > > > > > > > > > Your thoughts? Anyone familiar with any other projects' practices? > > > > > > > > > > Greg Trasuk. > > > > > > > > > > > > > > > On Fri, 2013-04-26 at 16:58, Peter Firmstone wrote: > > > > > > Checked RIVER-[404], the jtreg test certs still fail. > > > > > > > > > > > > It's been fixed in trunk, we need to remove it from the release > > > > > > notes > > > > > > for 2.2.1 > > > > > > > > > > > > I don't think River-[403] or River-[417] made it into 2.2.1 either. > > > > > > > > > > > > I think these issues were marked as completed as part of a previous > > > > > > release process that was left unfinished. > > > > > > > > > > > > River-[403] really needs to be included though. > > > > > > > > > > > > Regards, > > > > > > > > > > > > Peter. > > > > > > > > > > > > *** Start test: Sat Apr 27 06:46:43 EST 2013 > > > > > > [jtreg] Test 1: > > > > > >TestRMI$TestClientSubjectAfterSwitchConstraints: > > > > > > [jtreg] FAIL: Unexpected exception: > > > java.lang.IllegalStateException: > > > > > > no object exported via this exporter > > > > > > [jtreg] at > > > > > > net.jini.jeri.BasicJeriExporter.unexport(BasicJeriExporter.java:661) > > > > > > [jtreg] at > > > > > > TestRMI$TestClientSubjectAfterSwitchConstraints.run(TestRMI.java:182) > > > > > > [jtreg] at > > > > > >UnitTestUtilities.test(UnitTestUtilities.java:185) > > > > > > [jtreg] at > > > > > >UnitTestUtilities.test(UnitTestUtilities.java:130) > > > > > > [jtreg] at > > > > > >UnitTestUtilities.test(UnitTestUtilities.java:143) > > > > > > [jtreg] at > > > > > >UnitTestUtilities.test(UnitTestUtilities.java:99) > > > > > > [jtreg] at TestRMI.main(TestRMI.java:53) > > > > > > [jtreg] at > > > sun.reflect.NativeMethodAccessorImpl.invoke0(Native > > > > > > Method) > > > > > > [jtreg] at > > > > > > > > > > > > > > sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) > > > > > > [jtreg] at > > > > > > > > > > > > > > sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) > > > > > > [jtreg] at > > > > > >java.lang.reflect.Method.invoke(Method.java:585) > > > > > > [jtreg] at > > > > > > > > > com.sun.javatest.regtest.MainWrapper$MainThread.run(MainWrapper.java:94) > > > > > > [jtreg] at java.lang.Thread.run(Thread.java:595) > > > > > > [jtreg] > > > > > > [jtreg] Test 2: TestRMI$TestUnexportInServerImpl: force=true > > > > > > [jtreg] FAIL: Unexpected exception: > > > java.rmi.server.ExportException: > > > > > > listen failed; nested exception is: > > > > > > [jtreg] net.jini.io.UnsupportedConstraintException: > > > > > >Problem > > > with > > > > > > certificates: CN=serverDSA, C=US > > > > > > [jtreg] java.security.cert.CertificateExpiredException: > > > > > >NotAfter: > > > > > > Mon Sep 05 00:24:12 EST 2011 > > > > > > [jtreg] at > > > > > > > > > > > > > > com.sun.jini.jeri.internal.runtime.BasicExportTable.export(BasicExportTable.java:84) > > > > > > [jtreg] at > > > > > > net.jini.jeri.BasicJeriExporter.export(BasicJeriExporter.java:603) > > > > > > [jtreg] at > > > TestRMI$TestUnexportInServerImpl.run(TestRMI.java:240) > > > > > > [jtreg] at > > > > > >UnitTestUtilities.test(UnitTestUtilities.java:185) > > > > > > [jtreg] at > > > > > >UnitTestUtilities.test(UnitTestUtilities.java:130) > > > > > > [jtreg] at > > > > > >UnitTestUtilities.test(UnitTestUtilities.java:134) > > > > > > [jtreg] at > > > > > >UnitTestUtilities.test(UnitTestUtilities.java:143) > > > > > > [jtreg] at > > > > > >UnitTestUtilities.test(UnitTestUtilities.java:99) > > > > > > [jtreg] at TestRMI.main(TestRMI.java:53) > > > > > > [jtreg] at > > > sun.reflect.NativeMethodAccessorImpl.invoke0(Native > > > > > > Method) > > > > > > [jtreg] at > > > > > > > > > > > > > > sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) > > > > > > [jtreg] at > > > > > > > > > > > > > > sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) > > > > > > [jtreg] at > > > > > >java.lang.reflect.Method.invoke(Method.java:585) > > > > > > [jtreg] at > > > > > > > > > com.sun.javatest.regtest.MainWrapper$MainThread.run(MainWrapper.java:94) > > > > > > [jtreg] at java.lang.Thread.run(Thread.java:595) > > > > > > [jtreg] Caused by: > > > > > >net.jini.io.UnsupportedConstraintException: > > > > > > Problem with certificates: CN=serverDSA, C=US > > > > > > [jtreg] java.security.cert.CertificateExpiredException: > > > > > >NotAfter: > > > > > > Mon Sep 05 00:24:12 EST 2011 > > > > > > [jtreg] at > > > > > > > > > > > > > > net.jini.jeri.ssl.SslServerEndpointImpl$SslListenEndpoint.checkCredentials(SslServerEndpointImpl.java:726) > > > > > > [jtreg] at > > > > > > > > > > > > > > net.jini.jeri.ssl.SslServerEndpointImpl$SslListenEndpoint.listen(SslServerEndpointImpl.java:658) > > > > > > [jtreg] at > > > > > > com.sun.jini.jeri.internal.runtime.Binding$2.run(Binding.java:92) > > > > > > [jtreg] at > > > > > >java.security.AccessController.doPrivileged(Native > > > > > > Method) > > > > > > [jtreg] at > > > net.jini.security.Security$5.run(Security.java:543) > > > > > > [jtreg] at > > > > > >java.security.AccessController.doPrivileged(Native > > > > > > Method) > > > > > > [jtreg] at > > > > > > net.jini.security.Security.doPrivileged(Security.java:540) > > > > > > [jtreg] at > > > > > > com.sun.jini.jeri.internal.runtime.Binding.activate(Binding.java:89) > > > > > > [jtreg] at > > > > > > > > > > > > > > com.sun.jini.jeri.internal.runtime.BasicExportTable.getBinding(BasicExportTable.java:221) > > > > > > [jtreg] at > > > > > > > > > > > > > > com.sun.jini.jeri.internal.runtime.BasicExportTable.access$100(BasicExportTable.java:46) > > > > > > [jtreg] at > > > > > > > > > > > > > > com.sun.jini.jeri.internal.runtime.BasicExportTable$LC.addListenEndpoint(BasicExportTable.java:247) > > > > > > [jtreg] at > > > > > > > > > > > > > > net.jini.jeri.ssl.SslServerEndpointImpl.enumerateListenEndpoints(SslServerEndpointImpl.java:568) > > > > > > [jtreg] at > > > > > > > > > > > > > > net.jini.jeri.ssl.HttpsServerEndpoint.enumerateListenEndpoints(HttpsServerEndpoint.java:835) > > > > > > [jtreg] at > > > > > > > > > > > > > > com.sun.jini.jeri.internal.runtime.BasicExportTable.export(BasicExportTable.java:81) > > > > > > [jtreg] ... 14 more > > > > > > [jtreg] Caused by: > > > java.security.cert.CertificateExpiredException: > > > > > > NotAfter: Mon Sep 05 00:24:12 EST 2011 > > > > > > [jtreg] at > > > > > > > > > sun.security.x509.CertificateValidity.valid(CertificateValidity.java:256) > > > > > > [jtreg] at > > > > > > sun.security.x509.X509CertImpl.checkValidity(X509CertImpl.java:570) > > > > > > [jtreg] at > > > > > > sun.security.x509.X509CertImpl.checkValidity(X509CertImpl.java:543) > > > > > > [jtreg] at > > > > > > net.jini.jeri.ssl.Utilities.checkValidity(Utilities.java:774) > > > > > > [jtreg] at > > > > > > > > > > > > > > net.jini.jeri.ssl.SslServerEndpointImpl$SslListenEndpoint.checkCredentials(SslServerEndpointImpl.java:698) > > > > > > [jtreg] ... 27 more > > > > > > > > > > > > Greg Trasuk wrote: > > > > > > > Hi all: > > > > > > > > > > > > > > This build https://builds.apache.org/job/river-2.2-qa-jdk7/4/says > > > > > > > it > > > > > > > failed, but if you look closely at the console output, all the > > > testing > > > > > > > passed. There was a configuration error (mine) in the archiving > > > > > results > > > > > > > post-build step. There's another build scheduled which should show > > > a > > > > > > > complete "pass" result. Unfortunately, as we know, the test run > > > > > > > is > > > > > > > about 16 hours. And we're not the only project with long test > > > runs, so > > > > > > > there's a substantial wait time before the run gets onto an > > > executor > > > > > > > machine (just as an aside, I'm looking into setting up a Jenkins > > > > > > > instance of my own to run test builds in the future. Perhaps > > > others > > > > > > > should think of doing the same thing). In any case, given the > > > minimal > > > > > > > changes from 2.2, I'm now comfortable going forward with a > > > > > > > release. > > > > > > > > > > > > > > I'm currently building the 2.2.1 release candidate and am thinking > > > of > > > > > > > calling for a vote shortly. Should I go straight to the vote, or > > > do > > > > > > > people want to review and independently test the 2.2 branch first? > > > > > > > > > > > > > > Not that I'm calling a vote yet, but as I'm typing, I see the > > > release > > > > > > > candidate has finished uploading to > > > > > > > http://people.apache.org/~gtrasuk/river/, so if you want to have a > > > > > look > > > > > > > at it, go ahead, and let me know if there's any issues. I will > > > mention > > > > > > > that if you read the RAT reports, the "qa_jtreg" report names 224 > > > files > > > > > > > without license headers. These are ".policy" and other > > > configuration > > > > > > > files with little creative content. Further, they were in the > > > previous > > > > > > > release as approved by the Incubator, so I think we're on safe > > > ground > > > > > > > here, although they probably should have license headers added in > > > the > > > > > > > trunk at some point. > > > > > > > > > > > > > > Also, as a point of order, normally a vote would run for 72 hours. > > > > > > > Given that the weekend is coming up, I'm inclined to extend that > > > to 96 > > > > > > > hours. Opinions? > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > Greg Trasuk. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
