Seeking committers' help to review on SS PR

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Seeking committers' help to review on SS PR

Jungtaek Lim-2
Hi devs,

I have been struggling to find reviewers who are committers, to get my PR [1] for SPARK-27237 [2] reviewed. The PR was submitted on Mar. 2019 (1.5 years ago), and somehow it got two approvals from contributors working on the SS area, but still doesn't get any committers' traction to review.
(I can review others' SS PRs and I'm trying to unblock other SS area contributors, but I can't self review my SS PRs. Not sure it's technically possible, but fully sure it's not encouraged.)

Could I please ask help to unblock this before feature freeze for Spark 3.1 is happening? Submitted 1.5 years ago and continues struggling for including it in Spark 3.2 (another half of a year) doesn't make sense to me.

In addition, is there a way to unblock me to work for meaningful features instead of being stuck with small improvements? I have something in my backlog but I'd rather not want to continue struggling with new PRs.

Thanks,
Jungtaek Lim (HeartSaVioR)

Reply | Threaded
Open this post in threaded view
|

Re: Seeking committers' help to review on SS PR

Sean Owen-2
I don't see any objections on that thread. You're a committer and have reviews from other knowledgeable people in this area. Do you have any reason to believe it's controversial, like, changes semantics or APIs? Were there related discussions elsewhere that expressed any concern?

From a glance, OK it's introducing a new idea of state schema and validation; would it conflict with any other possible approaches, have any limits if this is enshrined as supported functionality? There's always some cost to introducing yet more code to support, but, this doesn't look intrusive or large.

The "don't review your own PR" idea isn't hard-and-fast. I don't think anyone needs to block for anything like this long if you have other capable reviews and you are a committer, if you don't see that it impacts other code meaningfully in a way that really demands review from others, and in good faith judge that it is worthwhile. I think you are the one de facto expert on that code and indeed you can't block yourself for 1.5 years or else nothing substantial would happen.



On Mon, Nov 23, 2020 at 1:18 AM Jungtaek Lim <[hidden email]> wrote:
Hi devs,

I have been struggling to find reviewers who are committers, to get my PR [1] for SPARK-27237 [2] reviewed. The PR was submitted on Mar. 2019 (1.5 years ago), and somehow it got two approvals from contributors working on the SS area, but still doesn't get any committers' traction to review.
(I can review others' SS PRs and I'm trying to unblock other SS area contributors, but I can't self review my SS PRs. Not sure it's technically possible, but fully sure it's not encouraged.)

Could I please ask help to unblock this before feature freeze for Spark 3.1 is happening? Submitted 1.5 years ago and continues struggling for including it in Spark 3.2 (another half of a year) doesn't make sense to me.

In addition, is there a way to unblock me to work for meaningful features instead of being stuck with small improvements? I have something in my backlog but I'd rather not want to continue struggling with new PRs.

Thanks,
Jungtaek Lim (HeartSaVioR)

Reply | Threaded
Open this post in threaded view
|

Re: Seeking committers' help to review on SS PR

Ryan Blue
I'll go take a look.

While I would generally agree with Sean that it would be appropriate in this case to commit, I'm very hesitant to set that precedent. I'd prefer to stick with "review then commit" and, if needed, relax that constraint for parts of the project that can't get reviewers for a certain period of time. We did that in another community where there weren't many reviewers and we wanted to get more people involved, but we put a time limit on it and set expectations to prevent any perception of abuse. I would support doing that in SS.

Thanks for being so patient on that PR. I'm sorry that you had to wait so long.

On Mon, Nov 23, 2020 at 7:11 AM Sean Owen <[hidden email]> wrote:
I don't see any objections on that thread. You're a committer and have reviews from other knowledgeable people in this area. Do you have any reason to believe it's controversial, like, changes semantics or APIs? Were there related discussions elsewhere that expressed any concern?

From a glance, OK it's introducing a new idea of state schema and validation; would it conflict with any other possible approaches, have any limits if this is enshrined as supported functionality? There's always some cost to introducing yet more code to support, but, this doesn't look intrusive or large.

The "don't review your own PR" idea isn't hard-and-fast. I don't think anyone needs to block for anything like this long if you have other capable reviews and you are a committer, if you don't see that it impacts other code meaningfully in a way that really demands review from others, and in good faith judge that it is worthwhile. I think you are the one de facto expert on that code and indeed you can't block yourself for 1.5 years or else nothing substantial would happen.



On Mon, Nov 23, 2020 at 1:18 AM Jungtaek Lim <[hidden email]> wrote:
Hi devs,

I have been struggling to find reviewers who are committers, to get my PR [1] for SPARK-27237 [2] reviewed. The PR was submitted on Mar. 2019 (1.5 years ago), and somehow it got two approvals from contributors working on the SS area, but still doesn't get any committers' traction to review.
(I can review others' SS PRs and I'm trying to unblock other SS area contributors, but I can't self review my SS PRs. Not sure it's technically possible, but fully sure it's not encouraged.)

Could I please ask help to unblock this before feature freeze for Spark 3.1 is happening? Submitted 1.5 years ago and continues struggling for including it in Spark 3.2 (another half of a year) doesn't make sense to me.

In addition, is there a way to unblock me to work for meaningful features instead of being stuck with small improvements? I have something in my backlog but I'd rather not want to continue struggling with new PRs.

Thanks,
Jungtaek Lim (HeartSaVioR)



--
Ryan Blue
Software Engineer
Netflix
Reply | Threaded
Open this post in threaded view
|

Re: Seeking committers' help to review on SS PR

Sean Owen-2
Yes, agree, and that time limit is probably a lot shorter than 1.5 years.
I think these ultimately come down to judgment, and am affirming the judgment that this amounts to 'reviewed'.

On Mon, Nov 23, 2020 at 11:40 AM Ryan Blue <[hidden email]> wrote:
I'll go take a look.

While I would generally agree with Sean that it would be appropriate in this case to commit, I'm very hesitant to set that precedent. I'd prefer to stick with "review then commit" and, if needed, relax that constraint for parts of the project that can't get reviewers for a certain period of time. We did that in another community where there weren't many reviewers and we wanted to get more people involved, but we put a time limit on it and set expectations to prevent any perception of abuse. I would support doing that in SS.

Thanks for being so patient on that PR. I'm sorry that you had to wait so long.

On Mon, Nov 23, 2020 at 7:11 AM Sean Owen <[hidden email]> wrote:
I don't see any objections on that thread. You're a committer and have reviews from other knowledgeable people in this area. Do you have any reason to believe it's controversial, like, changes semantics or APIs? Were there related discussions elsewhere that expressed any concern?

From a glance, OK it's introducing a new idea of state schema and validation; would it conflict with any other possible approaches, have any limits if this is enshrined as supported functionality? There's always some cost to introducing yet more code to support, but, this doesn't look intrusive or large.

The "don't review your own PR" idea isn't hard-and-fast. I don't think anyone needs to block for anything like this long if you have other capable reviews and you are a committer, if you don't see that it impacts other code meaningfully in a way that really demands review from others, and in good faith judge that it is worthwhile. I think you are the one de facto expert on that code and indeed you can't block yourself for 1.5 years or else nothing substantial would happen.



On Mon, Nov 23, 2020 at 1:18 AM Jungtaek Lim <[hidden email]> wrote:
Hi devs,

I have been struggling to find reviewers who are committers, to get my PR [1] for SPARK-27237 [2] reviewed. The PR was submitted on Mar. 2019 (1.5 years ago), and somehow it got two approvals from contributors working on the SS area, but still doesn't get any committers' traction to review.
(I can review others' SS PRs and I'm trying to unblock other SS area contributors, but I can't self review my SS PRs. Not sure it's technically possible, but fully sure it's not encouraged.)

Could I please ask help to unblock this before feature freeze for Spark 3.1 is happening? Submitted 1.5 years ago and continues struggling for including it in Spark 3.2 (another half of a year) doesn't make sense to me.

In addition, is there a way to unblock me to work for meaningful features instead of being stuck with small improvements? I have something in my backlog but I'd rather not want to continue struggling with new PRs.

Thanks,
Jungtaek Lim (HeartSaVioR)



--
Ryan Blue
Software Engineer
Netflix
Reply | Threaded
Open this post in threaded view
|

Re: Seeking committers' help to review on SS PR

Jungtaek Lim-2
Thanks for providing valuable feedback. Appreciate it. Sorry I haven't had time to reply to this in time (was OoO this week).

I'm also in favor of "review then commit", I haven't been a "perfect" guy making no mistake (probably that justifies me as a human being), hence the review process is a critical one I really would like to go through in any way. The problem is, it's less likely I could get attention for my SS PRs from "committers". Hopefully there are a couple of contributors in the SS area, so getting reviewed by itself is a bit easier than before, but in any way I couldn't get a finalized review and go merging.

This PR is actually an easy case, small enough and not invasive. I have planned major features most likely to bring design changes, which cannot go without attention from committers. I'd probably need committers for both design & code review - I'm a bit worried that such major change could be achieved with current situation.
(This may not be resolved even with the time limit... If the community has a faith to me then I could bravely just go with time limit, but, is this something I can avoid ending up being blamed?)

Probably the better resolution is filling enough SS committers. There should be some ways to do this, existing committers expert of SS area come back (which isn't something we can control), committers expert of other area jump in and help the area (which isn't also something we can control), or inviting new committer(s) for SS area. Anything would be fine for me.


On Tue, Nov 24, 2020 at 2:46 AM Sean Owen <[hidden email]> wrote:
Yes, agree, and that time limit is probably a lot shorter than 1.5 years.
I think these ultimately come down to judgment, and am affirming the judgment that this amounts to 'reviewed'.

On Mon, Nov 23, 2020 at 11:40 AM Ryan Blue <[hidden email]> wrote:
I'll go take a look.

While I would generally agree with Sean that it would be appropriate in this case to commit, I'm very hesitant to set that precedent. I'd prefer to stick with "review then commit" and, if needed, relax that constraint for parts of the project that can't get reviewers for a certain period of time. We did that in another community where there weren't many reviewers and we wanted to get more people involved, but we put a time limit on it and set expectations to prevent any perception of abuse. I would support doing that in SS.

Thanks for being so patient on that PR. I'm sorry that you had to wait so long.

On Mon, Nov 23, 2020 at 7:11 AM Sean Owen <[hidden email]> wrote:
I don't see any objections on that thread. You're a committer and have reviews from other knowledgeable people in this area. Do you have any reason to believe it's controversial, like, changes semantics or APIs? Were there related discussions elsewhere that expressed any concern?

From a glance, OK it's introducing a new idea of state schema and validation; would it conflict with any other possible approaches, have any limits if this is enshrined as supported functionality? There's always some cost to introducing yet more code to support, but, this doesn't look intrusive or large.

The "don't review your own PR" idea isn't hard-and-fast. I don't think anyone needs to block for anything like this long if you have other capable reviews and you are a committer, if you don't see that it impacts other code meaningfully in a way that really demands review from others, and in good faith judge that it is worthwhile. I think you are the one de facto expert on that code and indeed you can't block yourself for 1.5 years or else nothing substantial would happen.



On Mon, Nov 23, 2020 at 1:18 AM Jungtaek Lim <[hidden email]> wrote:
Hi devs,

I have been struggling to find reviewers who are committers, to get my PR [1] for SPARK-27237 [2] reviewed. The PR was submitted on Mar. 2019 (1.5 years ago), and somehow it got two approvals from contributors working on the SS area, but still doesn't get any committers' traction to review.
(I can review others' SS PRs and I'm trying to unblock other SS area contributors, but I can't self review my SS PRs. Not sure it's technically possible, but fully sure it's not encouraged.)

Could I please ask help to unblock this before feature freeze for Spark 3.1 is happening? Submitted 1.5 years ago and continues struggling for including it in Spark 3.2 (another half of a year) doesn't make sense to me.

In addition, is there a way to unblock me to work for meaningful features instead of being stuck with small improvements? I have something in my backlog but I'd rather not want to continue struggling with new PRs.

Thanks,
Jungtaek Lim (HeartSaVioR)



--
Ryan Blue
Software Engineer
Netflix
Reply | Threaded
Open this post in threaded view
|

Re: Seeking committers' help to review on SS PR

Jungtaek Lim-2
Btw, there are two more PRs which got LGTM by a SS contributor but fail to get attention from committers. They're 6+ months old. Could you help reviewing this as well, or do you all think 6 months of time range + LGTM from an SS contributor is enough to go ahead?


These are under 100 lines of changes per each, and not invasive.

On Sat, Nov 28, 2020 at 11:34 AM Jungtaek Lim <[hidden email]> wrote:
Thanks for providing valuable feedback. Appreciate it. Sorry I haven't had time to reply to this in time (was OoO this week).

I'm also in favor of "review then commit", I haven't been a "perfect" guy making no mistake (probably that justifies me as a human being), hence the review process is a critical one I really would like to go through in any way. The problem is, it's less likely I could get attention for my SS PRs from "committers". Hopefully there are a couple of contributors in the SS area, so getting reviewed by itself is a bit easier than before, but in any way I couldn't get a finalized review and go merging.

This PR is actually an easy case, small enough and not invasive. I have planned major features most likely to bring design changes, which cannot go without attention from committers. I'd probably need committers for both design & code review - I'm a bit worried that such major change could be achieved with current situation.
(This may not be resolved even with the time limit... If the community has a faith to me then I could bravely just go with time limit, but, is this something I can avoid ending up being blamed?)

Probably the better resolution is filling enough SS committers. There should be some ways to do this, existing committers expert of SS area come back (which isn't something we can control), committers expert of other area jump in and help the area (which isn't also something we can control), or inviting new committer(s) for SS area. Anything would be fine for me.


On Tue, Nov 24, 2020 at 2:46 AM Sean Owen <[hidden email]> wrote:
Yes, agree, and that time limit is probably a lot shorter than 1.5 years.
I think these ultimately come down to judgment, and am affirming the judgment that this amounts to 'reviewed'.

On Mon, Nov 23, 2020 at 11:40 AM Ryan Blue <[hidden email]> wrote:
I'll go take a look.

While I would generally agree with Sean that it would be appropriate in this case to commit, I'm very hesitant to set that precedent. I'd prefer to stick with "review then commit" and, if needed, relax that constraint for parts of the project that can't get reviewers for a certain period of time. We did that in another community where there weren't many reviewers and we wanted to get more people involved, but we put a time limit on it and set expectations to prevent any perception of abuse. I would support doing that in SS.

Thanks for being so patient on that PR. I'm sorry that you had to wait so long.

On Mon, Nov 23, 2020 at 7:11 AM Sean Owen <[hidden email]> wrote:
I don't see any objections on that thread. You're a committer and have reviews from other knowledgeable people in this area. Do you have any reason to believe it's controversial, like, changes semantics or APIs? Were there related discussions elsewhere that expressed any concern?

From a glance, OK it's introducing a new idea of state schema and validation; would it conflict with any other possible approaches, have any limits if this is enshrined as supported functionality? There's always some cost to introducing yet more code to support, but, this doesn't look intrusive or large.

The "don't review your own PR" idea isn't hard-and-fast. I don't think anyone needs to block for anything like this long if you have other capable reviews and you are a committer, if you don't see that it impacts other code meaningfully in a way that really demands review from others, and in good faith judge that it is worthwhile. I think you are the one de facto expert on that code and indeed you can't block yourself for 1.5 years or else nothing substantial would happen.



On Mon, Nov 23, 2020 at 1:18 AM Jungtaek Lim <[hidden email]> wrote:
Hi devs,

I have been struggling to find reviewers who are committers, to get my PR [1] for SPARK-27237 [2] reviewed. The PR was submitted on Mar. 2019 (1.5 years ago), and somehow it got two approvals from contributors working on the SS area, but still doesn't get any committers' traction to review.
(I can review others' SS PRs and I'm trying to unblock other SS area contributors, but I can't self review my SS PRs. Not sure it's technically possible, but fully sure it's not encouraged.)

Could I please ask help to unblock this before feature freeze for Spark 3.1 is happening? Submitted 1.5 years ago and continues struggling for including it in Spark 3.2 (another half of a year) doesn't make sense to me.

In addition, is there a way to unblock me to work for meaningful features instead of being stuck with small improvements? I have something in my backlog but I'd rather not want to continue struggling with new PRs.

Thanks,
Jungtaek Lim (HeartSaVioR)



--
Ryan Blue
Software Engineer
Netflix
Reply | Threaded
Open this post in threaded view
|

Re: Seeking committers' help to review on SS PR

Sean Owen-2
I don't know the code well, but those look minor and straightforward. They have reviews from the two most knowledgeable people in this area. I don't think you need to block for 6 months after proactively seeking all likely reviewers - I'm saying that's the resolution to this type of situation (too).

On Fri, Nov 27, 2020 at 8:55 PM Jungtaek Lim <[hidden email]> wrote:
Btw, there are two more PRs which got LGTM by a SS contributor but fail to get attention from committers. They're 6+ months old. Could you help reviewing this as well, or do you all think 6 months of time range + LGTM from an SS contributor is enough to go ahead?


These are under 100 lines of changes per each, and not invasive.
Reply | Threaded
Open this post in threaded view
|

Re: Seeking committers' help to review on SS PR

Ryan Blue
Jungtaek,

If there are contributors that you trust for reviews, then please let PMC members know so they can be considered. I agree that is the best solution.

If there aren't contributors that the PMC wants to add as committers, then I suggest agreeing on a temporary exception to help make progress in this area and give contributors more opportunities to develop. Something like this: for the next 6 months, contributions from committers to SS can be committed without a committer +1 if they are reviewed by at least one contributor (and have no dissent from committers, of course). Then after the period expires, we would ideally have new people ready to be added as committers.

That would need to be voted on, but I think it is a reasonable step to help resuscitate Spark streaming.

On Fri, Nov 27, 2020 at 7:15 PM Sean Owen <[hidden email]> wrote:
I don't know the code well, but those look minor and straightforward. They have reviews from the two most knowledgeable people in this area. I don't think you need to block for 6 months after proactively seeking all likely reviewers - I'm saying that's the resolution to this type of situation (too).

On Fri, Nov 27, 2020 at 8:55 PM Jungtaek Lim <[hidden email]> wrote:
Btw, there are two more PRs which got LGTM by a SS contributor but fail to get attention from committers. They're 6+ months old. Could you help reviewing this as well, or do you all think 6 months of time range + LGTM from an SS contributor is enough to go ahead?


These are under 100 lines of changes per each, and not invasive.


--
Ryan Blue
Software Engineer
Netflix
Reply | Threaded
Open this post in threaded view
|

Re: Seeking committers' help to review on SS PR

Xiao Li
Just want to say thank you to all the active SS contributors. I saw many great features/improvements in Streaming have been merged and will be available in the upcoming 3.1 release.
  • Cache fetched list of files beyond maxFilesPerTrigger as unread file (SPARK-32568)
  • Streamline the logic on file stream source and sink metadata log (SPARK-30462)
  • Add DataStreamReader.table API (SPARK-32885)
  • Add DataStreamWriter.saveAsTable API (SPARK-32896)
  • Left semi stream-stream join (SPARK-32862)
  • Introduce schema validation for streaming state store (SPARK-31894)
  • Support to use a different compression codec in state store (SPARK-33263)
  • Kafka connector infinite wait because metadata never updated (SPARK-28367)
  • Upgrade Kafka to 2.6.0 (SPARK-32568)
  • Pagination support for Structured Streaming UI pages (SPARK-31642, SPARK-30119)
  • State information in Structured Streaming UI (SPARK-33223)
Structured Streaming UI support in Spark History Server is another great usability feature: https://github.com/apache/spark/pull/28781 Hopefully, this can be part of 3.1 release. 

Go Spark!

Xiao



Ryan Blue <[hidden email]> 于2020年11月30日周一 上午11:35写道:
Jungtaek,

If there are contributors that you trust for reviews, then please let PMC members know so they can be considered. I agree that is the best solution.

If there aren't contributors that the PMC wants to add as committers, then I suggest agreeing on a temporary exception to help make progress in this area and give contributors more opportunities to develop. Something like this: for the next 6 months, contributions from committers to SS can be committed without a committer +1 if they are reviewed by at least one contributor (and have no dissent from committers, of course). Then after the period expires, we would ideally have new people ready to be added as committers.

That would need to be voted on, but I think it is a reasonable step to help resuscitate Spark streaming.

On Fri, Nov 27, 2020 at 7:15 PM Sean Owen <[hidden email]> wrote:
I don't know the code well, but those look minor and straightforward. They have reviews from the two most knowledgeable people in this area. I don't think you need to block for 6 months after proactively seeking all likely reviewers - I'm saying that's the resolution to this type of situation (too).

On Fri, Nov 27, 2020 at 8:55 PM Jungtaek Lim <[hidden email]> wrote:
Btw, there are two more PRs which got LGTM by a SS contributor but fail to get attention from committers. They're 6+ months old. Could you help reviewing this as well, or do you all think 6 months of time range + LGTM from an SS contributor is enough to go ahead?


These are under 100 lines of changes per each, and not invasive.


--
Ryan Blue
Software Engineer
Netflix