Re: [vpp-dev] 11th hour changes and code reviews
> On May 4, 2020, at 3:46 PM, Andrew Yourtchenko wrote: > More inline: >> On 4 May 2020, at 19:20, Paul Vinciguerra wrote: >> >> I agree that the change I submitted is a monster. Honestly, there are 2-5 >> git-friendly changes that could be refactored out of it. Its written and >> used by me, but it's not worth investing the time to make mergeable if there >> is no desire for it. > > That’s why it’s handy to discuss the big changes that touch the APIs upfront, > with the end users. Just last week I saw a mail on how the past year there > were so many API changes, which make life harder. VPP’s reason for existence > (at least in my view) is its users. > > This change reshuffles the messages and changes semantics for a bunch of > calls for the sole purpose of architectural purity. Purity is beautiful but > the users deserve a heads up at least. > >> I only submitted it at all because I had mentioned last week to someone >> that I had essentially stopped contributing and I was asked if I could be >> enticed to start re-submitting code. There is no intent on my part to >> subvert the release. >> >> Everyone complains about the tests. I was "asked" last year to clean up the >> tests. It's not that code hasn't been written to clean it up. The reality >> is that code becomes a monster when simple refactorings aren't merged. > > The tests need proper debugging first, *before* doing the refactorings. The > tests still can not run with high concurrent number of jobs. (N~48). > The tests still fail intermittently. > > This is what folks mean when when complaining. Rearranging the calling > convention doesn’t help this in the slightest, just adds unnecessary churn. > >> >> If you all tell me you want the changes, I'm glad to clean it up. > > That change contains multiple pieces which you yourself described. > > - dump by sw_if_index ? Sure. But again, not across the entire code base but > component by component (if there are many). Reviewed by the maintainer of the > component. The fact that this might be a bit painful to do in parts is a nice feedback to the pain that the change causes downstream. :) I like consistent APIs as much as the next person, maybe more so, but non-backward compatible changes in order to tidy up the API cause pain and maybe aren't so great. That said, adding filter functionality is good, so if there wasn't the ability to filter on sw_if_index before, I'd vote for it. Thanks, Chris. > > - changing the message structure ? Again, if there are multiple components, > it should not be hard making small changes that would be trivial to review > (though before that, like i said, it would be nice to prove the users and > subsystem maintainers what they think of the changes). Also how do we ensure > the “old” inconsistency doesn’t resurrect in the new messages ? If there is > no automated check against that, this is just adding churn. > > - internal refactorings that have zero user impact. Why not. But again, if > you are going across components, each commit should be within a component, > and reviewed/committed by the maintainer of the component. > > —a > >> >> If, in the meantime, someone wants to +2 Neale's change [2], that would be >> great! >> >> [0] https://gerrit.fd.io/r/c/vpp/+/26833 >> [1] https://lists.fd.io/g/csit-dev/message/4009 >> [2] https://gerrit.fd.io/r/c/vpp/+/26820/4 > -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16236): https://lists.fd.io/g/vpp-dev/message/16236 Mute This Topic: https://lists.fd.io/mt/73980355/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [vpp-dev] 11th hour changes and code reviews
Thanks for writing! Since i was the “-2“, first exec summary and then points inline: - CRC changing changes in 10 unrelated .api files at this point in time are borderline hostile activity to downstream users. Every CRC change means changes are required downstream. Sure it’s master but some folks are also preparing the releases dependent on VPP, and given the times I prefer to limit the changes that don’t correspond to obvious end-user-benefiting features. - as someone who has to do cherrypicks regularly, I can say that commits like this, which span multitude of files and yet include multiple orthogonal fixes are making this job much harder. This means that for any downstream commit that is unrelated to this but happens to touch one of the changes lines, I will need to open up the massive commit, and figure out which part does which, etc. More inline: >> On 4 May 2020, at 19:20, Paul Vinciguerra wrote: > I was -2'd this morning for submitting a change with a "blast radius" this > close to api freeze [0]. It was suggested that it should be discussed on the > dev list and that multiple parties outside of vpp should have to +1 it. I am > glad to start the conversation. > > My change is dependent on another developer's change has been sitting > waiting. for a response from csit-dev almost a week. I asked last week if it > would be ok to refactor out the non-csit related code, but got no response. > I refactored the dependent code out of the other change, but it is still > outstanding. That change probably is another api breaking change, I presume. The more api breaking changes are in the CSIT pipeline, the slower they will respond. But it’s speculation. We just did the 19.08.2 so they might be busy also testing that. > > I agree that the change I submitted is a monster. Honestly, there are 2-5 > git-friendly changes that could be refactored out of it. Its written and > used by me, but it's not worth investing the time to make mergeable if there > is no desire for it. That’s why it’s handy to discuss the big changes that touch the APIs upfront, with the end users. Just last week I saw a mail on how the past year there were so many API changes, which make life harder. VPP’s reason for existence (at least in my view) is its users. This change reshuffles the messages and changes semantics for a bunch of calls for the sole purpose of architectural purity. Purity is beautiful but the users deserve a heads up at least. > I only submitted it at all because I had mentioned last week to someone > that I had essentially stopped contributing and I was asked if I could be > enticed to start re-submitting code. There is no intent on my part to > subvert the release. > > Everyone complains about the tests. I was "asked" last year to clean up the > tests. It's not that code hasn't been written to clean it up. The reality > is that code becomes a monster when simple refactorings aren't merged. The tests need proper debugging first, *before* doing the refactorings. The tests still can not run with high concurrent number of jobs. (N~48). The tests still fail intermittently. This is what folks mean when when complaining. Rearranging the calling convention doesn’t help this in the slightest, just adds unnecessary churn. > > If you all tell me you want the changes, I'm glad to clean it up. That change contains multiple pieces which you yourself described. - dump by sw_if_index ? Sure. But again, not across the entire code base but component by component (if there are many). Reviewed by the maintainer of the component. - changing the message structure ? Again, if there are multiple components, it should not be hard making small changes that would be trivial to review (though before that, like i said, it would be nice to prove the users and subsystem maintainers what they think of the changes). Also how do we ensure the “old” inconsistency doesn’t resurrect in the new messages ? If there is no automated check against that, this is just adding churn. - internal refactorings that have zero user impact. Why not. But again, if you are going across components, each commit should be within a component, and reviewed/committed by the maintainer of the component. —a > > If, in the meantime, someone wants to +2 Neale's change [2], that would be > great! > > [0] https://gerrit.fd.io/r/c/vpp/+/26833 > [1] https://lists.fd.io/g/csit-dev/message/4009 > [2] https://gerrit.fd.io/r/c/vpp/+/26820/4 -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16234): https://lists.fd.io/g/vpp-dev/message/16234 Mute This Topic: https://lists.fd.io/mt/73980355/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[vpp-dev] 11th hour changes and code reviews
I was -2'd this morning for submitting a change with a "blast radius" this close to api freeze [0]. It was suggested that it should be discussed on the dev list and that multiple parties outside of vpp should have to +1 it. I am glad to start the conversation. My change is dependent on another developer's change has been sitting waiting. for a response from csit-dev almost a week. I asked last week if it would be ok to refactor out the non-csit related code, but got no response. I refactored the dependent code out of the other change, but it is still outstanding. I agree that the change I submitted is a monster. Honestly, there are 2-5 git-friendly changes that could be refactored out of it. Its written and used by me, but it's not worth investing the time to make mergeable if there is no desire for it. I only submitted it at all because I had mentioned last week to someone that I had essentially stopped contributing and I was asked if I could be enticed to start re-submitting code. There is no intent on my part to subvert the release. Everyone complains about the tests. I was "asked" last year to clean up the tests. It's not that code hasn't been written to clean it up. The reality is that code becomes a monster when simple refactorings aren't merged. If you all tell me you want the changes, I'm glad to clean it up. If, in the meantime, someone wants to +2 Neale's change [2], that would be great! [0] https://gerrit.fd.io/r/c/vpp/+/26833 ( https://gerrit.fd.io/r/c/vpp/+/26833 ) [1] https://lists.fd.io/g/csit-dev/message/4009 ( https://lists.fd.io/g/csit-dev/message/4009 ) [2] https://gerrit.fd.io/r/c/vpp/+/26820/4 -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16231): https://lists.fd.io/g/vpp-dev/message/16231 Mute This Topic: https://lists.fd.io/mt/73980355/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
