A new protocol for thockin PR reviews
167 views
Skip to first unread message
Tim Hockin
Nov 30, 2022, 6:54:10 PM11/30/22
to dev, kubernetes-sig-network
Hi everyone,
I’ve been thinking about and talking about this for a while, and I
wanted to write it down ASAP. It got a little long, sorry.
The current cycle of KEP freeze, code freeze, repeat is not
sustainable for me. For a multitude of reasons, it always comes down
to the last 2 weeks of each cycle being _ridiculously_ busy. I know I
am not alone in this, but if I am being honest, it is becoming
increasingly difficult for me to manage while also having other
responsibilities.
Here’s what I plan to do about it.
PR reviews (mostly code but also KEPs) usually take non-linear time
compared to their size. I usually make it a personal policy to review
PRs in order of their size - XS first, XXL last. This is partly to
encourage small PRs but also because I can usually squeeze a few small
and even medium sized PRs into gaps in my daily calendar. Large (and
XLarge, and XXLarge) sized PRs generally take _much_ longer (hours to
days) and I need to *make* time for those.
Because of this, the bigger PRs tend to get delayed reviews, which
means we push up against the deadlines, which creates stress (for
myself and PR authors), and sometimes (too often) we miss the deadline
and have to start over in a new cycle. This stinks for everyone
involved.
The obvious answer is smaller PRs. Sometimes this is possible, but
sometimes it’s not. I am a vocal proponent of NOT merging half of a
feature (e.g. API without implementation). For new APIs or
substantial changes, that automatically means large PRs. One approach
that can help is to aggressively use multiple commits to optimize for
review (it’s hard to say exactly what this means - it really depends
on the PR).
I am instituting some new policies _for myself_ with regards to PRs,
and I wanted to let people know ahead of the next cycle(s). Other
maintainers may choose to follow suit, and maybe we’ll make it a
project norm eventually, but for now, this is what I am imposing on
myself.
0) I will always try to service smaller PRs before larger ones.
The best way to get my review is to keep PRs small and well-organized.
PLEASE use commits to illustrate independent actions (e.g. “this
commit just renames things”, “this commit refactors”, “this commit
adds new functionality”) and, even better, try to break things like
refactoring and cleanups into distinct PRs. It’s WAY easier for me to
review 10 small PRs than 1 large one.
1) Just because I reviewed (or approved) your KEP doesn’t necessarily
mean I am signing up for the code review.
If you think your PR is going to be big and you want me to review it,
I need you to confirm with me that I am signed up for that. If I say
yes, I will do my very best to follow through on it. I will try to be
realistic and not say yes to too many things, but if you drop an
unplanned jumbo PR on me, I can’t promise that I will be able to
review it in time.
2) Big PRs (especially those that cross SIGs) need multiple reviewers.
Kubernetes is large and nobody is an expert in all areas (least of all
me). If you want me to review your jumbo PR, I need you to show me
who else is signed up to review (or at least who you are asking) and
what area(s) they are covering. Putting together a review team should
happen relatively early in the life cycle of large changes.
TBD: should the cross-SIG “review team” be part of the KEP becoming
“implementable”?
TBD: should “estimated size” be part of the KEP?
3) Big PRs should be reviewed incrementally.
We’ve experimented with “feature branches”, but I’m open to trying
simpler approaches. If you want me to review the code that will
ultimately add up to a jumbo PR, let’s agree on a process which allows
me to review the work _as it is happening_. For example, create a
fork repo and send PRs to it, so that I (and the review team) can
participate earlier than “it’s all done”.
For the things I sign up to review, I will offer regular and ad hoc
meeting “slots”, so we can jump on a video call and discuss
incremental changes and plans in real time, well before the deadline.
This requires that you, the author, do not procrastinate. If you save
it all up for the end, I can’t promise that I will be able to review
it in time.
Hopefully these changes will bring a little sanity to the chaos, and
result in less contributions missing the boat, at least not because of
me :)
Tim
I’ve been thinking about and talking about this for a while, and I
wanted to write it down ASAP. It got a little long, sorry.
The current cycle of KEP freeze, code freeze, repeat is not
sustainable for me. For a multitude of reasons, it always comes down
to the last 2 weeks of each cycle being _ridiculously_ busy. I know I
am not alone in this, but if I am being honest, it is becoming
increasingly difficult for me to manage while also having other
responsibilities.
Here’s what I plan to do about it.
PR reviews (mostly code but also KEPs) usually take non-linear time
compared to their size. I usually make it a personal policy to review
PRs in order of their size - XS first, XXL last. This is partly to
encourage small PRs but also because I can usually squeeze a few small
and even medium sized PRs into gaps in my daily calendar. Large (and
XLarge, and XXLarge) sized PRs generally take _much_ longer (hours to
days) and I need to *make* time for those.
Because of this, the bigger PRs tend to get delayed reviews, which
means we push up against the deadlines, which creates stress (for
myself and PR authors), and sometimes (too often) we miss the deadline
and have to start over in a new cycle. This stinks for everyone
involved.
The obvious answer is smaller PRs. Sometimes this is possible, but
sometimes it’s not. I am a vocal proponent of NOT merging half of a
feature (e.g. API without implementation). For new APIs or
substantial changes, that automatically means large PRs. One approach
that can help is to aggressively use multiple commits to optimize for
review (it’s hard to say exactly what this means - it really depends
on the PR).
I am instituting some new policies _for myself_ with regards to PRs,
and I wanted to let people know ahead of the next cycle(s). Other
maintainers may choose to follow suit, and maybe we’ll make it a
project norm eventually, but for now, this is what I am imposing on
myself.
0) I will always try to service smaller PRs before larger ones.
The best way to get my review is to keep PRs small and well-organized.
PLEASE use commits to illustrate independent actions (e.g. “this
commit just renames things”, “this commit refactors”, “this commit
adds new functionality”) and, even better, try to break things like
refactoring and cleanups into distinct PRs. It’s WAY easier for me to
review 10 small PRs than 1 large one.
1) Just because I reviewed (or approved) your KEP doesn’t necessarily
mean I am signing up for the code review.
If you think your PR is going to be big and you want me to review it,
I need you to confirm with me that I am signed up for that. If I say
yes, I will do my very best to follow through on it. I will try to be
realistic and not say yes to too many things, but if you drop an
unplanned jumbo PR on me, I can’t promise that I will be able to
review it in time.
2) Big PRs (especially those that cross SIGs) need multiple reviewers.
Kubernetes is large and nobody is an expert in all areas (least of all
me). If you want me to review your jumbo PR, I need you to show me
who else is signed up to review (or at least who you are asking) and
what area(s) they are covering. Putting together a review team should
happen relatively early in the life cycle of large changes.
TBD: should the cross-SIG “review team” be part of the KEP becoming
“implementable”?
TBD: should “estimated size” be part of the KEP?
3) Big PRs should be reviewed incrementally.
We’ve experimented with “feature branches”, but I’m open to trying
simpler approaches. If you want me to review the code that will
ultimately add up to a jumbo PR, let’s agree on a process which allows
me to review the work _as it is happening_. For example, create a
fork repo and send PRs to it, so that I (and the review team) can
participate earlier than “it’s all done”.
For the things I sign up to review, I will offer regular and ad hoc
meeting “slots”, so we can jump on a video call and discuss
incremental changes and plans in real time, well before the deadline.
This requires that you, the author, do not procrastinate. If you save
it all up for the end, I can’t promise that I will be able to review
it in time.
Hopefully these changes will bring a little sanity to the chaos, and
result in less contributions missing the boat, at least not because of
me :)
Tim
Aravindh Puthiyaparambil
Dec 1, 2022, 11:07:51 AM12/1/22
to dev, tho...@google.com, kubernetes-sig-network
Hi TIm,
Thanks for writing this up and it is hugely helpful to folks hoping to get reviews from you.
How would like us to get the confirmation? On the PR itself? Is there a way for us to see your current list of PRs so that we don't have to bother you with confirmation again?
Cheers,
Aravindh
Kevin Klues
Dec 1, 2022, 11:20:53 AM12/1/22
to arav...@redhat.com, dev, tho...@google.com
I've been using a personal github board to manage my review queue.
You can click "Add item" at the bottom of the column you would like to add a PR or issue to and paste the URL directly in.
It has helped me immensely in keeping track of which PRs I am tracking, as well as giving others visibility into whether their PRs are on my radar or not.
May be useful for you as well.
Kevin
--
You received this message because you are subscribed to the Google Groups "dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to dev+uns...@kubernetes.io.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%40kubernetes.io.
~Kevin
Fox, Kevin M
Dec 1, 2022, 12:01:23 PM12/1/22
to dev, arav...@redhat.com, tho...@google.com
A couple of thoughts. With vendoring, and code generation, minor features can gain a lot of bulk in size of change set. Could the labels be updated to take that into account so what really is a small change is marked such to get better prioritization?
The other thing that might help, which may be time to discuss is how frequently major releases are being made. 3 a year is starting to become excessive imo. As a user, the number of significant changes that I really need to put into production asap is greatly diminishing as time goes on. The nice to haves are going up. I think this speaks to the maturity of Kubernetes at this point. This means maybe 2 or even 1 major release a year wouldn't hurt me too much I think. Might even give me more time to deal with other things. I'm guessing at least some of the developers would have the same feeling about getting some time back?
Thanks,
Kevin
________________________________________
From: Aravindh Puthiyaparambil <arav...@redhat.com>
Sent: Thursday, December 1, 2022 8:07 AM
To: dev
Cc: tho...@google.com; kubernetes-sig-network
Subject: Re: A new protocol for thockin PR reviews
Check twice before you click! This email originated from outside PNNL.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%40kubernetes.io<https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2F8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%2540kubernetes.io%3Futm_medium%3Demail%26utm_source%3Dfooter&data=05%7C01%7CKevin.Fox%40pnnl.gov%7C9a1e762a4c94424fa0b608dad3b639ba%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055076857524909%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=C%2BBA%2FACW9T2GbF3C4j7Nhd9HvzHkMvfRWC7fD4uHFbw%3D&reserved=0>.
The other thing that might help, which may be time to discuss is how frequently major releases are being made. 3 a year is starting to become excessive imo. As a user, the number of significant changes that I really need to put into production asap is greatly diminishing as time goes on. The nice to haves are going up. I think this speaks to the maturity of Kubernetes at this point. This means maybe 2 or even 1 major release a year wouldn't hurt me too much I think. Might even give me more time to deal with other things. I'm guessing at least some of the developers would have the same feeling about getting some time back?
Thanks,
Kevin
________________________________________
From: Aravindh Puthiyaparambil <arav...@redhat.com>
Sent: Thursday, December 1, 2022 8:07 AM
To: dev
Cc: tho...@google.com; kubernetes-sig-network
Subject: Re: A new protocol for thockin PR reviews
Check twice before you click! This email originated from outside PNNL.
--
You received this message because you are subscribed to the Google Groups "dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to dev+uns...@kubernetes.io<mailto:dev+uns...@kubernetes.io>.
You received this message because you are subscribed to the Google Groups "dev" group.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%40kubernetes.io<https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2F8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%2540kubernetes.io%3Futm_medium%3Demail%26utm_source%3Dfooter&data=05%7C01%7CKevin.Fox%40pnnl.gov%7C9a1e762a4c94424fa0b608dad3b639ba%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055076857524909%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=C%2BBA%2FACW9T2GbF3C4j7Nhd9HvzHkMvfRWC7fD4uHFbw%3D&reserved=0>.
Aldo Culquicondor
Dec 1, 2022, 1:29:15 PM12/1/22
to kevi...@pnnl.gov, dev, arav...@redhat.com, tho...@google.com
> 3 a year is starting to become excessive imo. As a user, the number of significant changes that I really need to put into production asap is greatly diminishing as time goes on. The nice to haves are going up.
I don't think this can be generalized. While the maturity remark might apply to stateless applications, I don't think the same can be said for stateful or batch, and probably other domains. 1 release per year would heavily delay awaited features that we are working on.
Aldo
To unsubscribe from this group and stop receiving emails from it, send an email to dev+uns...@kubernetes.io.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/CO6PR09MB74488D25F6C0C7FC7A43C86796149%40CO6PR09MB7448.namprd09.prod.outlook.com.
Jordan Liggitt
Dec 1, 2022, 1:35:28 PM12/1/22
to aco...@google.com, kevi...@pnnl.gov, dev, arav...@redhat.com, tho...@google.com
The releases-per-year point is interesting, but we should probably have that discussion in a dedicated thread. Rather than slowing the cadence, I'd be more interested in seeing how we could make life easier for consumers who are happy with our GA functionality, and are only upgrading to stay on supported versions, not for latest/greatest/in-development features.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/CAJi5r2oGRxVGTko8Jrppqt2juyWKjP85anCRkQTKVP2T_g02%3Dw%40mail.gmail.com.
Fox, Kevin M
Dec 1, 2022, 1:51:37 PM12/1/22
to Jordan Liggitt, aco...@google.com, dev, arav...@redhat.com, tho...@google.com
The other option I can think of like that would be to support one level of skip upgrades? so something like you can always upgrade between even releases without going through the odd ones. That would still allow 3 a year for those that need it, while people who are satisfied with existing functionality and wanted to keep up could do so with a 1.5 per year, 1 every 8 month, cadence. That negates any benefits on the developer side though. So I agree, that probably should be discussed on a separate thread if that is more desirable.
I am looking forward to better batch support too. So I do get the desire for that. Same with deployments + configmap/secret orchestration being made more intuitive. If it helped the developers not burn out and we only got 2 releases a year, just saying, it maybe worth the tradeoff imo.
Thanks,
Kevin
________________________________________
From: Jordan Liggitt <lig...@google.com>
Sent: Thursday, December 1, 2022 10:35 AM
To: aco...@google.com
Cc: Fox, Kevin M; dev; arav...@redhat.com; tho...@google.com
To unsubscribe from this group and stop receiving emails from it, send an email to dev+uns...@kubernetes.io<mailto:dev%2Bunsu...@kubernetes.io><mailto:dev+uns...@kubernetes.io<mailto:dev%2Bunsu...@kubernetes.io>>.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%40kubernetes.io<https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2F8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%2540kubernetes.io&data=05%7C01%7Ckevin.fox%40pnnl.gov%7C74f21cab4d39418f313108dad3cacd38%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055165232744163%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=xqNJeGt1Nh15MVbCn6W3sQZLD3Y3MaYdp4L0q0FZAJ8%3D&reserved=0><https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2F8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%2540kubernetes.io%3Futm_medium%3Demail%26utm_source%3Dfooter&data=05%7C01%7CKevin.Fox%40pnnl.gov%7C9a1e762a4c94424fa0b608dad3b639ba%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055076857524909%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=C%2BBA%2FACW9T2GbF3C4j7Nhd9HvzHkMvfRWC7fD4uHFbw%3D&reserved=0<https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2F8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%2540kubernetes.io%3Futm_medium%3Demail%26utm_source%3Dfooter&data=05%7C01%7Ckevin.fox%40pnnl.gov%7C74f21cab4d39418f313108dad3cacd38%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055165232744163%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=bEx10H6uG%2Ba%2Bcau%2BRrbrseU5xCTpsx3IQTOpvsJv%2FBU%3D&reserved=0>>.
--
You received this message because you are subscribed to the Google Groups "dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to dev+uns...@kubernetes.io<mailto:dev%2Bunsu...@kubernetes.io>.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/CO6PR09MB74488D25F6C0C7FC7A43C86796149%40CO6PR09MB7448.namprd09.prod.outlook.com<https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2FCO6PR09MB74488D25F6C0C7FC7A43C86796149%2540CO6PR09MB7448.namprd09.prod.outlook.com&data=05%7C01%7Ckevin.fox%40pnnl.gov%7C74f21cab4d39418f313108dad3cacd38%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055165232744163%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=7qLq3IrDveSVXm%2FOxhrNXyHHL%2FmfgPwZSuMBLnY0QGI%3D&reserved=0>.
--
You received this message because you are subscribed to the Google Groups "dev" group.
I am looking forward to better batch support too. So I do get the desire for that. Same with deployments + configmap/secret orchestration being made more intuitive. If it helped the developers not burn out and we only got 2 releases a year, just saying, it maybe worth the tradeoff imo.
Thanks,
Kevin
________________________________________
From: Jordan Liggitt <lig...@google.com>
Sent: Thursday, December 1, 2022 10:35 AM
To: aco...@google.com
Cc: Fox, Kevin M; dev; arav...@redhat.com; tho...@google.com
Subject: Re: A new protocol for thockin PR reviews
The releases-per-year point is interesting, but we should probably have that discussion in a dedicated thread. Rather than slowing the cadence, I'd be more interested in seeing how we could make life easier for consumers who are happy with our GA functionality, and are only upgrading to stay on supported versions, not for latest/greatest/in-development features.
On Thu, Dec 1, 2022 at 1:29 PM 'Aldo Culquicondor' via dev <d...@kubernetes.io<mailto:d...@kubernetes.io>> wrote:
> 3 a year is starting to become excessive imo. As a user, the number of significant changes that I really need to put into production asap is greatly diminishing as time goes on. The nice to haves are going up.
I don't think this can be generalized. While the maturity remark might apply to stateless applications, I don't think the same can be said for stateful or batch, and probably other domains. 1 release per year would heavily delay awaited features that we are working on.
Aldo
> 3 a year is starting to become excessive imo. As a user, the number of significant changes that I really need to put into production asap is greatly diminishing as time goes on. The nice to haves are going up.
I don't think this can be generalized. While the maturity remark might apply to stateless applications, I don't think the same can be said for stateful or batch, and probably other domains. 1 release per year would heavily delay awaited features that we are working on.
Aldo
On Thu, Dec 1, 2022 at 12:01 PM 'Fox, Kevin M' via dev <d...@kubernetes.io<mailto:d...@kubernetes.io>> wrote:
A couple of thoughts. With vendoring, and code generation, minor features can gain a lot of bulk in size of change set. Could the labels be updated to take that into account so what really is a small change is marked such to get better prioritization?
The other thing that might help, which may be time to discuss is how frequently major releases are being made. 3 a year is starting to become excessive imo. As a user, the number of significant changes that I really need to put into production asap is greatly diminishing as time goes on. The nice to haves are going up. I think this speaks to the maturity of Kubernetes at this point. This means maybe 2 or even 1 major release a year wouldn't hurt me too much I think. Might even give me more time to deal with other things. I'm guessing at least some of the developers would have the same feeling about getting some time back?
Thanks,
Kevin
________________________________________
From: Aravindh Puthiyaparambil <arav...@redhat.com<mailto:arav...@redhat.com>>
A couple of thoughts. With vendoring, and code generation, minor features can gain a lot of bulk in size of change set. Could the labels be updated to take that into account so what really is a small change is marked such to get better prioritization?
The other thing that might help, which may be time to discuss is how frequently major releases are being made. 3 a year is starting to become excessive imo. As a user, the number of significant changes that I really need to put into production asap is greatly diminishing as time goes on. The nice to haves are going up. I think this speaks to the maturity of Kubernetes at this point. This means maybe 2 or even 1 major release a year wouldn't hurt me too much I think. Might even give me more time to deal with other things. I'm guessing at least some of the developers would have the same feeling about getting some time back?
Thanks,
Kevin
________________________________________
Sent: Thursday, December 1, 2022 8:07 AM
To: dev
Cc: tho...@google.com<mailto:tho...@google.com>; kubernetes-sig-network
To: dev
Subject: Re: A new protocol for thockin PR reviews
Check twice before you click! This email originated from outside PNNL.
Hi TIm,
Thanks for writing this up and it is hugely helpful to folks hoping to get reviews from you.
Check twice before you click! This email originated from outside PNNL.
Hi TIm,
Thanks for writing this up and it is hugely helpful to folks hoping to get reviews from you.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%40kubernetes.io<https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2F8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%2540kubernetes.io&data=05%7C01%7Ckevin.fox%40pnnl.gov%7C74f21cab4d39418f313108dad3cacd38%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055165232744163%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=xqNJeGt1Nh15MVbCn6W3sQZLD3Y3MaYdp4L0q0FZAJ8%3D&reserved=0><https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2F8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%2540kubernetes.io%3Futm_medium%3Demail%26utm_source%3Dfooter&data=05%7C01%7CKevin.Fox%40pnnl.gov%7C9a1e762a4c94424fa0b608dad3b639ba%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055076857524909%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=C%2BBA%2FACW9T2GbF3C4j7Nhd9HvzHkMvfRWC7fD4uHFbw%3D&reserved=0<https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2F8afa02b9-417b-4e0d-b673-4a5b1d1112c3n%2540kubernetes.io%3Futm_medium%3Demail%26utm_source%3Dfooter&data=05%7C01%7Ckevin.fox%40pnnl.gov%7C74f21cab4d39418f313108dad3cacd38%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055165232744163%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=bEx10H6uG%2Ba%2Bcau%2BRrbrseU5xCTpsx3IQTOpvsJv%2FBU%3D&reserved=0>>.
--
You received this message because you are subscribed to the Google Groups "dev" group.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/CO6PR09MB74488D25F6C0C7FC7A43C86796149%40CO6PR09MB7448.namprd09.prod.outlook.com<https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2FCO6PR09MB74488D25F6C0C7FC7A43C86796149%2540CO6PR09MB7448.namprd09.prod.outlook.com&data=05%7C01%7Ckevin.fox%40pnnl.gov%7C74f21cab4d39418f313108dad3cacd38%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055165232744163%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=7qLq3IrDveSVXm%2FOxhrNXyHHL%2FmfgPwZSuMBLnY0QGI%3D&reserved=0>.
--
You received this message because you are subscribed to the Google Groups "dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to dev+uns...@kubernetes.io<mailto:dev+uns...@kubernetes.io>.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/CAJi5r2oGRxVGTko8Jrppqt2juyWKjP85anCRkQTKVP2T_g02%3Dw%40mail.gmail.com<https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fa%2Fkubernetes.io%2Fd%2Fmsgid%2Fdev%2FCAJi5r2oGRxVGTko8Jrppqt2juyWKjP85anCRkQTKVP2T_g02%253Dw%2540mail.gmail.com%3Futm_medium%3Demail%26utm_source%3Dfooter&data=05%7C01%7Ckevin.fox%40pnnl.gov%7C74f21cab4d39418f313108dad3cacd38%7Cd6faa5f90ae240338c0130048a38deeb%7C0%7C0%7C638055165232744163%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5r04uU4pcoZgMJj9XVjgRPhYL%2FNdztV1kWuIwOSMDFQ%3D&reserved=0>.
Vinay Kulkarni
Dec 1, 2022, 3:51:37 PM12/1/22
to dev, tho...@google.com, kubernetes-sig-network
Thanks for this great writeup, Tim.
As someone who is guilty of sticking Tim, Derek, and others with a multi-SIG, multi-component XL PR (102884) and disappointing them with 1.26 release miss, I've been wondering what I could have done better to ease the pain for reviewers and for retro disucssion. I want to add some color/specifics that I hope is useful.
- Zero functionality change refactoring can help reduce size of PR. Some changes with #102884 as example are adding helper functions or splitting up a big function. Since unit tests are usually part of this change, it adds up, and it is a rebase pain. These changes can be justified as 'going to need it for feature X' imho. Examples:
- Adding GetIndexOfContainerStatus function could have been a separate PR.
- Adding GetCgroupConfig, SetCgroupConfig, etc + tests and related code could have been its own PR.
- Early in this PR, generateLinuxContainerConfig was a big function. Refactoring some of the code in that function into generateLinuxContainerResources + UT could have been a separate PR.
- +1 for scheduled/adhoc review meetings. It is a lot easier to sync and discuss rationale for a change / major code-structuring f2f and course correct instead of async via github comments.
- I would add a comment blurb briefly explaining the code-flow at key flow entry points - it would save a lot of effort just to get reviewers to understand why some change was made.
- Our thinking about all-or-none change has evolved a bit. We originally had two KEPS for this PR - one for core implementation, and one for CRI changes. But we always felt that it should be one change, but ended up extracting and committing CRI changes separately in the nick of time for 1.25 code freeze to resolve a chicken-egg problem and allow CRI support to come in during 1.26 release. So, don't be spineless like me - make a case and argue for it if you feel you have a valid point on case-by-case basis :)
Hope this helps.
Thanks,
Vinay
Tim Hockin
Dec 1, 2022, 4:06:36 PM12/1/22
to arav...@redhat.com, dev
On Thu, Dec 1, 2022 at 8:07 AM Aravindh Puthiyaparambil
<arav...@redhat.com> wrote:
>> 1) Just because I reviewed (or approved) your KEP doesn’t necessarily
>> mean I am signing up for the code review.
>>
>> If you think your PR is going to be big and you want me to review it,
>> I need you to confirm with me that I am signed up for that.
>
>
> How would like us to get the confirmation? On the PR itself? Is there a way for us to see your current list of PRs so that we don't have to bother you with confirmation again?
Great question! Anything is fine - slack, Github comments, etc. I'm
<arav...@redhat.com> wrote:
>> 1) Just because I reviewed (or approved) your KEP doesn’t necessarily
>> mean I am signing up for the code review.
>>
>> If you think your PR is going to be big and you want me to review it,
>> I need you to confirm with me that I am signed up for that.
>
>
> How would like us to get the confirmation? On the PR itself? Is there a way for us to see your current list of PRs so that we don't have to bother you with confirmation again?
going to look into a more transparent way to manage this all, but for
now, let's just make sure we communicate :)
Tim Hockin
Dec 1, 2022, 4:17:02 PM12/1/22
to Fox, Kevin M, dev, arav...@redhat.com
On Thu, Dec 1, 2022 at 8:49 AM Fox, Kevin M <Kevi...@pnnl.gov> wrote:
>
> A couple of thoughts. With vendoring, and code generation, minor features can gain a lot of bulk in size of change set. Could the labels be updated to take that into account so what really is a small change is marked such to get better prioritization?
I thought we did already consider a lot of these, but maybe there's
>
> A couple of thoughts. With vendoring, and code generation, minor features can gain a lot of bulk in size of change set. Could the labels be updated to take that into account so what really is a small change is marked such to get better prioritization?
more to do. That said, vendoring DOES need review and generated code
gets at least spot-checks :)
> The other thing that might help, which may be time to discuss is how frequently major releases are being made. 3 a year is starting to become excessive imo. As a user, the number of significant changes that I really need to put into production asap is greatly diminishing as time goes on. The nice to haves are going up. I think this speaks to the maturity of Kubernetes at this point. This means maybe 2 or even 1 major release a year wouldn't hurt me too much I think. Might even give me more time to deal with other things. I'm guessing at least some of the developers would have the same feeling about getting some time back?
myself* (whether it has a benefit for customers is a different
question). There's a certain amount of feature work being done -
whether we release it all once per year or once per month, the work
being done is the same. The main difference would probably be the
amount of extra work we save or add to go through the act of managing
releases.
Tim
Reply all
Reply to author
Forward
0 new messages