You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Michael Carter <mi...@instaclustr.com> on 2020/06/15 00:52:23 UTC
First time patch submitter advice
Hi all,
I’ve submitted a patch for the first time(https://github.com/apache/kafka/pull/8844 <https://github.com/apache/kafka/pull/8844>), and I have a couple of questions that I’m hoping someone can help me answer.
I’m a little unclear what happens after that patch has been submitted. The coding guidelines say Jenkins will run tests automatically, but I don’t see any results anywhere. Have I misunderstood what should happen, or do I just not know where to look?
Should I be attempting to find reviewers for the change myself, or is that done independently of the patch submitter?
Also, in resolving a couple of conflicts that have arisen after the patch was first submitted, I noticed that there are now failing unit tests that have nothing to do with my change. Is there a convention on how to deal with these? Should it be something that I try to fix on my branch?
Any thoughts are appreciated.
Thanks,
Michael
Re: First time patch submitter advice
Posted by Sohil Shah <bu...@gmail.com>.
Hope the committer runs the tests locally. If the Jenkins tests don’t work,
that is a back and forth between the team and committer. For the most part
if tested locally, the Kafka team is responsible to take the PR forward
from there on.
I really don’t understand the issue here
On Sun, Jun 14, 2020 at 7:52 PM Michael Carter <
michael.carter@instaclustr.com> wrote:
> Hi all,
>
> I’ve submitted a patch for the first time(
> https://github.com/apache/kafka/pull/8844 <
> https://github.com/apache/kafka/pull/8844>), and I have a couple of
> questions that I’m hoping someone can help me answer.
>
> I’m a little unclear what happens after that patch has been submitted. The
> coding guidelines say Jenkins will run tests automatically, but I don’t see
> any results anywhere. Have I misunderstood what should happen, or do I just
> not know where to look?
> Should I be attempting to find reviewers for the change myself, or is that
> done independently of the patch submitter?
>
> Also, in resolving a couple of conflicts that have arisen after the patch
> was first submitted, I noticed that there are now failing unit tests that
> have nothing to do with my change. Is there a convention on how to deal
> with these? Should it be something that I try to fix on my branch?
>
> Any thoughts are appreciated.
>
> Thanks,
> Michael
Re: First time patch submitter advice
Posted by Michael Carter <mi...@instaclustr.com>.
Thanks Bruno
> On 19 Jun 2020, at 6:48 pm, Bruno Cadonna <br...@confluent.io> wrote:
>
> I meant "Hi Michael" not Luke.
> Sorry Michael and Luke.
>
> Best,
> Bruno
>
> On Fri, Jun 19, 2020 at 10:47 AM Bruno Cadonna <br...@confluent.io> wrote:
>>
>> Hi Luke,
>>
>> The guide is a bit outdated. Thank you for pointing it out. I updated the guide.
>>
>> As Gwen stated above:
>>
>>> Unfortunately, you need to get a committer to approve running the tests.
>>
>> So, yes a committer has to comment on the PR.
>>
>> Best,
>> Bruno
>>
>> On Fri, Jun 19, 2020 at 1:28 AM Michael Carter
>> <mi...@instaclustr.com> wrote:
>>>
>>> Hi Gwen and Luke,
>>>
>>> Sorry, I’ve probably misunderstood something again. Since KAFKA-10155 and KAFKA-10147 have now been resolved, I merged the trunk back into my branch and added to comment “retest this please” to my pull request (https://github.com/apache/kafka/pull/8844 <https://github.com/apache/kafka/pull/8844>) like the contributing guidelines state. Unfortunately, no tests seem to have been run.
>>> Does a committer have to comment on the PR instead?
>>>
>>> Thanks,
>>> Michael
>>>
>>>> On 16 Jun 2020, at 9:33 am, Michael Carter <mi...@instaclustr.com> wrote:
>>>>
>>>> Great, thanks Luke.
>>>> I’ve undone the patch and added that comment.
>>>>
>>>> Cheers,
>>>> Michael
>>>>
>>>>> On 15 Jun 2020, at 6:07 pm, Luke Chen <sh...@gmail.com> wrote:
>>>>>
>>>>> Hi Michael,
>>>>> The failed unit test has already handled here:
>>>>> https://issues.apache.org/jira/browse/KAFKA-10155
>>>>> https://issues.apache.org/jira/browse/KAFKA-10147
>>>>>
>>>>> So, maybe you can ignore the test errors and mention the issue number in PR.
>>>>> Thanks.
>>>>>
>>>>> Luke
>>>>>
>>>>> On Mon, Jun 15, 2020 at 3:23 PM Michael Carter <
>>>>> michael.carter@instaclustr.com> wrote:
>>>>>
>>>>>> Thanks for the response Gwen, that clarifies things for me.
>>>>>>
>>>>>> Regarding the unit test (ReassignPartitionsUnitTest.
>>>>>> testModifyBrokerThrottles), it appears to fail quite reliably on trunk as
>>>>>> well (at least on my machine).
>>>>>> It looks to me like a new override to
>>>>>> MockAdminClient.describeConfigs(Collection<ConfigResource> resources)
>>>>>> (MockAdminClient.java line 369) introduced in commit
>>>>>> 48b56e533b3ff22ae0e2cf7fcc649e7df19f2b06 changed the behaviour of this
>>>>>> method that the unit test relied on.
>>>>>> I’ve just now put a patch into my branch to make that test pass by calling
>>>>>> a slightly different version of describeConfigs (that avoids the overridden
>>>>>> behaviour). It’s probably arguable whether that constitutes a fix or not
>>>>>> though.
>>>>>>
>>>>>> Cheers,
>>>>>> Michael
>>>>>>
>>>>>>> On 15 Jun 2020, at 3:41 pm, Gwen Shapira <gw...@confluent.io> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> 1. Unfortunately, you need to get a committer to approve running the
>>>>>> tests.
>>>>>>> I just gave the green-light on your PR.
>>>>>>> 2. You can hope that committers will see your PR, but sometimes things
>>>>>> get
>>>>>>> lost. If you know someone who is familiar with that area of the code, it
>>>>>> is
>>>>>>> a good idea to ping them.
>>>>>>> 3. We do have some flaky tests. You can see that Jenkins will run 3
>>>>>>> parallel builds, if some of them pass and the committer confirms that
>>>>>>> failures are not related to your code, we are ok to merge. Obviously, if
>>>>>>> you end up tracking them down and fixing, everyone will be very grateful.
>>>>>>>
>>>>>>> Hope this helps,
>>>>>>>
>>>>>>> Gwen
>>>>>>>
>>>>>>> On Sun, Jun 14, 2020 at 5:52 PM Michael Carter <
>>>>>>> michael.carter@instaclustr.com> wrote:
>>>>>>>
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> I’ve submitted a patch for the first time(
>>>>>>>> https://github.com/apache/kafka/pull/8844 <
>>>>>>>> https://github.com/apache/kafka/pull/8844>), and I have a couple of
>>>>>>>> questions that I’m hoping someone can help me answer.
>>>>>>>>
>>>>>>>> I’m a little unclear what happens after that patch has been submitted.
>>>>>> The
>>>>>>>> coding guidelines say Jenkins will run tests automatically, but I don’t
>>>>>> see
>>>>>>>> any results anywhere. Have I misunderstood what should happen, or do I
>>>>>> just
>>>>>>>> not know where to look?
>>>>>>>> Should I be attempting to find reviewers for the change myself, or is
>>>>>> that
>>>>>>>> done independently of the patch submitter?
>>>>>>>>
>>>>>>>> Also, in resolving a couple of conflicts that have arisen after the
>>>>>> patch
>>>>>>>> was first submitted, I noticed that there are now failing unit tests
>>>>>> that
>>>>>>>> have nothing to do with my change. Is there a convention on how to deal
>>>>>>>> with these? Should it be something that I try to fix on my branch?
>>>>>>>>
>>>>>>>> Any thoughts are appreciated.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Michael
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Gwen Shapira
>>>>>>> Engineering Manager | Confluent
>>>>>>> 650.450.2760 | @gwenshap
>>>>>>> Follow us: Twitter | blog
>>>>>>
>>>>>>
>>>>
>>>
Re: First time patch submitter advice
Posted by Bruno Cadonna <br...@confluent.io>.
I meant "Hi Michael" not Luke.
Sorry Michael and Luke.
Best,
Bruno
On Fri, Jun 19, 2020 at 10:47 AM Bruno Cadonna <br...@confluent.io> wrote:
>
> Hi Luke,
>
> The guide is a bit outdated. Thank you for pointing it out. I updated the guide.
>
> As Gwen stated above:
>
> > Unfortunately, you need to get a committer to approve running the tests.
>
> So, yes a committer has to comment on the PR.
>
> Best,
> Bruno
>
> On Fri, Jun 19, 2020 at 1:28 AM Michael Carter
> <mi...@instaclustr.com> wrote:
> >
> > Hi Gwen and Luke,
> >
> > Sorry, I’ve probably misunderstood something again. Since KAFKA-10155 and KAFKA-10147 have now been resolved, I merged the trunk back into my branch and added to comment “retest this please” to my pull request (https://github.com/apache/kafka/pull/8844 <https://github.com/apache/kafka/pull/8844>) like the contributing guidelines state. Unfortunately, no tests seem to have been run.
> > Does a committer have to comment on the PR instead?
> >
> > Thanks,
> > Michael
> >
> > > On 16 Jun 2020, at 9:33 am, Michael Carter <mi...@instaclustr.com> wrote:
> > >
> > > Great, thanks Luke.
> > > I’ve undone the patch and added that comment.
> > >
> > > Cheers,
> > > Michael
> > >
> > >> On 15 Jun 2020, at 6:07 pm, Luke Chen <sh...@gmail.com> wrote:
> > >>
> > >> Hi Michael,
> > >> The failed unit test has already handled here:
> > >> https://issues.apache.org/jira/browse/KAFKA-10155
> > >> https://issues.apache.org/jira/browse/KAFKA-10147
> > >>
> > >> So, maybe you can ignore the test errors and mention the issue number in PR.
> > >> Thanks.
> > >>
> > >> Luke
> > >>
> > >> On Mon, Jun 15, 2020 at 3:23 PM Michael Carter <
> > >> michael.carter@instaclustr.com> wrote:
> > >>
> > >>> Thanks for the response Gwen, that clarifies things for me.
> > >>>
> > >>> Regarding the unit test (ReassignPartitionsUnitTest.
> > >>> testModifyBrokerThrottles), it appears to fail quite reliably on trunk as
> > >>> well (at least on my machine).
> > >>> It looks to me like a new override to
> > >>> MockAdminClient.describeConfigs(Collection<ConfigResource> resources)
> > >>> (MockAdminClient.java line 369) introduced in commit
> > >>> 48b56e533b3ff22ae0e2cf7fcc649e7df19f2b06 changed the behaviour of this
> > >>> method that the unit test relied on.
> > >>> I’ve just now put a patch into my branch to make that test pass by calling
> > >>> a slightly different version of describeConfigs (that avoids the overridden
> > >>> behaviour). It’s probably arguable whether that constitutes a fix or not
> > >>> though.
> > >>>
> > >>> Cheers,
> > >>> Michael
> > >>>
> > >>>> On 15 Jun 2020, at 3:41 pm, Gwen Shapira <gw...@confluent.io> wrote:
> > >>>>
> > >>>> Hi,
> > >>>>
> > >>>> 1. Unfortunately, you need to get a committer to approve running the
> > >>> tests.
> > >>>> I just gave the green-light on your PR.
> > >>>> 2. You can hope that committers will see your PR, but sometimes things
> > >>> get
> > >>>> lost. If you know someone who is familiar with that area of the code, it
> > >>> is
> > >>>> a good idea to ping them.
> > >>>> 3. We do have some flaky tests. You can see that Jenkins will run 3
> > >>>> parallel builds, if some of them pass and the committer confirms that
> > >>>> failures are not related to your code, we are ok to merge. Obviously, if
> > >>>> you end up tracking them down and fixing, everyone will be very grateful.
> > >>>>
> > >>>> Hope this helps,
> > >>>>
> > >>>> Gwen
> > >>>>
> > >>>> On Sun, Jun 14, 2020 at 5:52 PM Michael Carter <
> > >>>> michael.carter@instaclustr.com> wrote:
> > >>>>
> > >>>>> Hi all,
> > >>>>>
> > >>>>> I’ve submitted a patch for the first time(
> > >>>>> https://github.com/apache/kafka/pull/8844 <
> > >>>>> https://github.com/apache/kafka/pull/8844>), and I have a couple of
> > >>>>> questions that I’m hoping someone can help me answer.
> > >>>>>
> > >>>>> I’m a little unclear what happens after that patch has been submitted.
> > >>> The
> > >>>>> coding guidelines say Jenkins will run tests automatically, but I don’t
> > >>> see
> > >>>>> any results anywhere. Have I misunderstood what should happen, or do I
> > >>> just
> > >>>>> not know where to look?
> > >>>>> Should I be attempting to find reviewers for the change myself, or is
> > >>> that
> > >>>>> done independently of the patch submitter?
> > >>>>>
> > >>>>> Also, in resolving a couple of conflicts that have arisen after the
> > >>> patch
> > >>>>> was first submitted, I noticed that there are now failing unit tests
> > >>> that
> > >>>>> have nothing to do with my change. Is there a convention on how to deal
> > >>>>> with these? Should it be something that I try to fix on my branch?
> > >>>>>
> > >>>>> Any thoughts are appreciated.
> > >>>>>
> > >>>>> Thanks,
> > >>>>> Michael
> > >>>>
> > >>>>
> > >>>>
> > >>>> --
> > >>>> Gwen Shapira
> > >>>> Engineering Manager | Confluent
> > >>>> 650.450.2760 | @gwenshap
> > >>>> Follow us: Twitter | blog
> > >>>
> > >>>
> > >
> >
Re: First time patch submitter advice
Posted by Bruno Cadonna <br...@confluent.io>.
Hi Luke,
The guide is a bit outdated. Thank you for pointing it out. I updated the guide.
As Gwen stated above:
> Unfortunately, you need to get a committer to approve running the tests.
So, yes a committer has to comment on the PR.
Best,
Bruno
On Fri, Jun 19, 2020 at 1:28 AM Michael Carter
<mi...@instaclustr.com> wrote:
>
> Hi Gwen and Luke,
>
> Sorry, I’ve probably misunderstood something again. Since KAFKA-10155 and KAFKA-10147 have now been resolved, I merged the trunk back into my branch and added to comment “retest this please” to my pull request (https://github.com/apache/kafka/pull/8844 <https://github.com/apache/kafka/pull/8844>) like the contributing guidelines state. Unfortunately, no tests seem to have been run.
> Does a committer have to comment on the PR instead?
>
> Thanks,
> Michael
>
> > On 16 Jun 2020, at 9:33 am, Michael Carter <mi...@instaclustr.com> wrote:
> >
> > Great, thanks Luke.
> > I’ve undone the patch and added that comment.
> >
> > Cheers,
> > Michael
> >
> >> On 15 Jun 2020, at 6:07 pm, Luke Chen <sh...@gmail.com> wrote:
> >>
> >> Hi Michael,
> >> The failed unit test has already handled here:
> >> https://issues.apache.org/jira/browse/KAFKA-10155
> >> https://issues.apache.org/jira/browse/KAFKA-10147
> >>
> >> So, maybe you can ignore the test errors and mention the issue number in PR.
> >> Thanks.
> >>
> >> Luke
> >>
> >> On Mon, Jun 15, 2020 at 3:23 PM Michael Carter <
> >> michael.carter@instaclustr.com> wrote:
> >>
> >>> Thanks for the response Gwen, that clarifies things for me.
> >>>
> >>> Regarding the unit test (ReassignPartitionsUnitTest.
> >>> testModifyBrokerThrottles), it appears to fail quite reliably on trunk as
> >>> well (at least on my machine).
> >>> It looks to me like a new override to
> >>> MockAdminClient.describeConfigs(Collection<ConfigResource> resources)
> >>> (MockAdminClient.java line 369) introduced in commit
> >>> 48b56e533b3ff22ae0e2cf7fcc649e7df19f2b06 changed the behaviour of this
> >>> method that the unit test relied on.
> >>> I’ve just now put a patch into my branch to make that test pass by calling
> >>> a slightly different version of describeConfigs (that avoids the overridden
> >>> behaviour). It’s probably arguable whether that constitutes a fix or not
> >>> though.
> >>>
> >>> Cheers,
> >>> Michael
> >>>
> >>>> On 15 Jun 2020, at 3:41 pm, Gwen Shapira <gw...@confluent.io> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> 1. Unfortunately, you need to get a committer to approve running the
> >>> tests.
> >>>> I just gave the green-light on your PR.
> >>>> 2. You can hope that committers will see your PR, but sometimes things
> >>> get
> >>>> lost. If you know someone who is familiar with that area of the code, it
> >>> is
> >>>> a good idea to ping them.
> >>>> 3. We do have some flaky tests. You can see that Jenkins will run 3
> >>>> parallel builds, if some of them pass and the committer confirms that
> >>>> failures are not related to your code, we are ok to merge. Obviously, if
> >>>> you end up tracking them down and fixing, everyone will be very grateful.
> >>>>
> >>>> Hope this helps,
> >>>>
> >>>> Gwen
> >>>>
> >>>> On Sun, Jun 14, 2020 at 5:52 PM Michael Carter <
> >>>> michael.carter@instaclustr.com> wrote:
> >>>>
> >>>>> Hi all,
> >>>>>
> >>>>> I’ve submitted a patch for the first time(
> >>>>> https://github.com/apache/kafka/pull/8844 <
> >>>>> https://github.com/apache/kafka/pull/8844>), and I have a couple of
> >>>>> questions that I’m hoping someone can help me answer.
> >>>>>
> >>>>> I’m a little unclear what happens after that patch has been submitted.
> >>> The
> >>>>> coding guidelines say Jenkins will run tests automatically, but I don’t
> >>> see
> >>>>> any results anywhere. Have I misunderstood what should happen, or do I
> >>> just
> >>>>> not know where to look?
> >>>>> Should I be attempting to find reviewers for the change myself, or is
> >>> that
> >>>>> done independently of the patch submitter?
> >>>>>
> >>>>> Also, in resolving a couple of conflicts that have arisen after the
> >>> patch
> >>>>> was first submitted, I noticed that there are now failing unit tests
> >>> that
> >>>>> have nothing to do with my change. Is there a convention on how to deal
> >>>>> with these? Should it be something that I try to fix on my branch?
> >>>>>
> >>>>> Any thoughts are appreciated.
> >>>>>
> >>>>> Thanks,
> >>>>> Michael
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> Gwen Shapira
> >>>> Engineering Manager | Confluent
> >>>> 650.450.2760 | @gwenshap
> >>>> Follow us: Twitter | blog
> >>>
> >>>
> >
>
Re: First time patch submitter advice
Posted by Michael Carter <mi...@instaclustr.com>.
Hi Gwen and Luke,
Sorry, I’ve probably misunderstood something again. Since KAFKA-10155 and KAFKA-10147 have now been resolved, I merged the trunk back into my branch and added to comment “retest this please” to my pull request (https://github.com/apache/kafka/pull/8844 <https://github.com/apache/kafka/pull/8844>) like the contributing guidelines state. Unfortunately, no tests seem to have been run.
Does a committer have to comment on the PR instead?
Thanks,
Michael
> On 16 Jun 2020, at 9:33 am, Michael Carter <mi...@instaclustr.com> wrote:
>
> Great, thanks Luke.
> I’ve undone the patch and added that comment.
>
> Cheers,
> Michael
>
>> On 15 Jun 2020, at 6:07 pm, Luke Chen <sh...@gmail.com> wrote:
>>
>> Hi Michael,
>> The failed unit test has already handled here:
>> https://issues.apache.org/jira/browse/KAFKA-10155
>> https://issues.apache.org/jira/browse/KAFKA-10147
>>
>> So, maybe you can ignore the test errors and mention the issue number in PR.
>> Thanks.
>>
>> Luke
>>
>> On Mon, Jun 15, 2020 at 3:23 PM Michael Carter <
>> michael.carter@instaclustr.com> wrote:
>>
>>> Thanks for the response Gwen, that clarifies things for me.
>>>
>>> Regarding the unit test (ReassignPartitionsUnitTest.
>>> testModifyBrokerThrottles), it appears to fail quite reliably on trunk as
>>> well (at least on my machine).
>>> It looks to me like a new override to
>>> MockAdminClient.describeConfigs(Collection<ConfigResource> resources)
>>> (MockAdminClient.java line 369) introduced in commit
>>> 48b56e533b3ff22ae0e2cf7fcc649e7df19f2b06 changed the behaviour of this
>>> method that the unit test relied on.
>>> I’ve just now put a patch into my branch to make that test pass by calling
>>> a slightly different version of describeConfigs (that avoids the overridden
>>> behaviour). It’s probably arguable whether that constitutes a fix or not
>>> though.
>>>
>>> Cheers,
>>> Michael
>>>
>>>> On 15 Jun 2020, at 3:41 pm, Gwen Shapira <gw...@confluent.io> wrote:
>>>>
>>>> Hi,
>>>>
>>>> 1. Unfortunately, you need to get a committer to approve running the
>>> tests.
>>>> I just gave the green-light on your PR.
>>>> 2. You can hope that committers will see your PR, but sometimes things
>>> get
>>>> lost. If you know someone who is familiar with that area of the code, it
>>> is
>>>> a good idea to ping them.
>>>> 3. We do have some flaky tests. You can see that Jenkins will run 3
>>>> parallel builds, if some of them pass and the committer confirms that
>>>> failures are not related to your code, we are ok to merge. Obviously, if
>>>> you end up tracking them down and fixing, everyone will be very grateful.
>>>>
>>>> Hope this helps,
>>>>
>>>> Gwen
>>>>
>>>> On Sun, Jun 14, 2020 at 5:52 PM Michael Carter <
>>>> michael.carter@instaclustr.com> wrote:
>>>>
>>>>> Hi all,
>>>>>
>>>>> I’ve submitted a patch for the first time(
>>>>> https://github.com/apache/kafka/pull/8844 <
>>>>> https://github.com/apache/kafka/pull/8844>), and I have a couple of
>>>>> questions that I’m hoping someone can help me answer.
>>>>>
>>>>> I’m a little unclear what happens after that patch has been submitted.
>>> The
>>>>> coding guidelines say Jenkins will run tests automatically, but I don’t
>>> see
>>>>> any results anywhere. Have I misunderstood what should happen, or do I
>>> just
>>>>> not know where to look?
>>>>> Should I be attempting to find reviewers for the change myself, or is
>>> that
>>>>> done independently of the patch submitter?
>>>>>
>>>>> Also, in resolving a couple of conflicts that have arisen after the
>>> patch
>>>>> was first submitted, I noticed that there are now failing unit tests
>>> that
>>>>> have nothing to do with my change. Is there a convention on how to deal
>>>>> with these? Should it be something that I try to fix on my branch?
>>>>>
>>>>> Any thoughts are appreciated.
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>>
>>>>
>>>>
>>>> --
>>>> Gwen Shapira
>>>> Engineering Manager | Confluent
>>>> 650.450.2760 | @gwenshap
>>>> Follow us: Twitter | blog
>>>
>>>
>
Re: First time patch submitter advice
Posted by Michael Carter <mi...@instaclustr.com>.
Great, thanks Luke.
I’ve undone the patch and added that comment.
Cheers,
Michael
> On 15 Jun 2020, at 6:07 pm, Luke Chen <sh...@gmail.com> wrote:
>
> Hi Michael,
> The failed unit test has already handled here:
> https://issues.apache.org/jira/browse/KAFKA-10155
> https://issues.apache.org/jira/browse/KAFKA-10147
>
> So, maybe you can ignore the test errors and mention the issue number in PR.
> Thanks.
>
> Luke
>
> On Mon, Jun 15, 2020 at 3:23 PM Michael Carter <
> michael.carter@instaclustr.com> wrote:
>
>> Thanks for the response Gwen, that clarifies things for me.
>>
>> Regarding the unit test (ReassignPartitionsUnitTest.
>> testModifyBrokerThrottles), it appears to fail quite reliably on trunk as
>> well (at least on my machine).
>> It looks to me like a new override to
>> MockAdminClient.describeConfigs(Collection<ConfigResource> resources)
>> (MockAdminClient.java line 369) introduced in commit
>> 48b56e533b3ff22ae0e2cf7fcc649e7df19f2b06 changed the behaviour of this
>> method that the unit test relied on.
>> I’ve just now put a patch into my branch to make that test pass by calling
>> a slightly different version of describeConfigs (that avoids the overridden
>> behaviour). It’s probably arguable whether that constitutes a fix or not
>> though.
>>
>> Cheers,
>> Michael
>>
>>> On 15 Jun 2020, at 3:41 pm, Gwen Shapira <gw...@confluent.io> wrote:
>>>
>>> Hi,
>>>
>>> 1. Unfortunately, you need to get a committer to approve running the
>> tests.
>>> I just gave the green-light on your PR.
>>> 2. You can hope that committers will see your PR, but sometimes things
>> get
>>> lost. If you know someone who is familiar with that area of the code, it
>> is
>>> a good idea to ping them.
>>> 3. We do have some flaky tests. You can see that Jenkins will run 3
>>> parallel builds, if some of them pass and the committer confirms that
>>> failures are not related to your code, we are ok to merge. Obviously, if
>>> you end up tracking them down and fixing, everyone will be very grateful.
>>>
>>> Hope this helps,
>>>
>>> Gwen
>>>
>>> On Sun, Jun 14, 2020 at 5:52 PM Michael Carter <
>>> michael.carter@instaclustr.com> wrote:
>>>
>>>> Hi all,
>>>>
>>>> I’ve submitted a patch for the first time(
>>>> https://github.com/apache/kafka/pull/8844 <
>>>> https://github.com/apache/kafka/pull/8844>), and I have a couple of
>>>> questions that I’m hoping someone can help me answer.
>>>>
>>>> I’m a little unclear what happens after that patch has been submitted.
>> The
>>>> coding guidelines say Jenkins will run tests automatically, but I don’t
>> see
>>>> any results anywhere. Have I misunderstood what should happen, or do I
>> just
>>>> not know where to look?
>>>> Should I be attempting to find reviewers for the change myself, or is
>> that
>>>> done independently of the patch submitter?
>>>>
>>>> Also, in resolving a couple of conflicts that have arisen after the
>> patch
>>>> was first submitted, I noticed that there are now failing unit tests
>> that
>>>> have nothing to do with my change. Is there a convention on how to deal
>>>> with these? Should it be something that I try to fix on my branch?
>>>>
>>>> Any thoughts are appreciated.
>>>>
>>>> Thanks,
>>>> Michael
>>>
>>>
>>>
>>> --
>>> Gwen Shapira
>>> Engineering Manager | Confluent
>>> 650.450.2760 | @gwenshap
>>> Follow us: Twitter | blog
>>
>>
Re: First time patch submitter advice
Posted by Luke Chen <sh...@gmail.com>.
Hi Michael,
The failed unit test has already handled here:
https://issues.apache.org/jira/browse/KAFKA-10155
https://issues.apache.org/jira/browse/KAFKA-10147
So, maybe you can ignore the test errors and mention the issue number in PR.
Thanks.
Luke
On Mon, Jun 15, 2020 at 3:23 PM Michael Carter <
michael.carter@instaclustr.com> wrote:
> Thanks for the response Gwen, that clarifies things for me.
>
> Regarding the unit test (ReassignPartitionsUnitTest.
> testModifyBrokerThrottles), it appears to fail quite reliably on trunk as
> well (at least on my machine).
> It looks to me like a new override to
> MockAdminClient.describeConfigs(Collection<ConfigResource> resources)
> (MockAdminClient.java line 369) introduced in commit
> 48b56e533b3ff22ae0e2cf7fcc649e7df19f2b06 changed the behaviour of this
> method that the unit test relied on.
> I’ve just now put a patch into my branch to make that test pass by calling
> a slightly different version of describeConfigs (that avoids the overridden
> behaviour). It’s probably arguable whether that constitutes a fix or not
> though.
>
> Cheers,
> Michael
>
> > On 15 Jun 2020, at 3:41 pm, Gwen Shapira <gw...@confluent.io> wrote:
> >
> > Hi,
> >
> > 1. Unfortunately, you need to get a committer to approve running the
> tests.
> > I just gave the green-light on your PR.
> > 2. You can hope that committers will see your PR, but sometimes things
> get
> > lost. If you know someone who is familiar with that area of the code, it
> is
> > a good idea to ping them.
> > 3. We do have some flaky tests. You can see that Jenkins will run 3
> > parallel builds, if some of them pass and the committer confirms that
> > failures are not related to your code, we are ok to merge. Obviously, if
> > you end up tracking them down and fixing, everyone will be very grateful.
> >
> > Hope this helps,
> >
> > Gwen
> >
> > On Sun, Jun 14, 2020 at 5:52 PM Michael Carter <
> > michael.carter@instaclustr.com> wrote:
> >
> >> Hi all,
> >>
> >> I’ve submitted a patch for the first time(
> >> https://github.com/apache/kafka/pull/8844 <
> >> https://github.com/apache/kafka/pull/8844>), and I have a couple of
> >> questions that I’m hoping someone can help me answer.
> >>
> >> I’m a little unclear what happens after that patch has been submitted.
> The
> >> coding guidelines say Jenkins will run tests automatically, but I don’t
> see
> >> any results anywhere. Have I misunderstood what should happen, or do I
> just
> >> not know where to look?
> >> Should I be attempting to find reviewers for the change myself, or is
> that
> >> done independently of the patch submitter?
> >>
> >> Also, in resolving a couple of conflicts that have arisen after the
> patch
> >> was first submitted, I noticed that there are now failing unit tests
> that
> >> have nothing to do with my change. Is there a convention on how to deal
> >> with these? Should it be something that I try to fix on my branch?
> >>
> >> Any thoughts are appreciated.
> >>
> >> Thanks,
> >> Michael
> >
> >
> >
> > --
> > Gwen Shapira
> > Engineering Manager | Confluent
> > 650.450.2760 | @gwenshap
> > Follow us: Twitter | blog
>
>
Re: First time patch submitter advice
Posted by Michael Carter <mi...@instaclustr.com>.
Thanks for the response Gwen, that clarifies things for me.
Regarding the unit test (ReassignPartitionsUnitTest. testModifyBrokerThrottles), it appears to fail quite reliably on trunk as well (at least on my machine).
It looks to me like a new override to MockAdminClient.describeConfigs(Collection<ConfigResource> resources) (MockAdminClient.java line 369) introduced in commit 48b56e533b3ff22ae0e2cf7fcc649e7df19f2b06 changed the behaviour of this method that the unit test relied on.
I’ve just now put a patch into my branch to make that test pass by calling a slightly different version of describeConfigs (that avoids the overridden behaviour). It’s probably arguable whether that constitutes a fix or not though.
Cheers,
Michael
> On 15 Jun 2020, at 3:41 pm, Gwen Shapira <gw...@confluent.io> wrote:
>
> Hi,
>
> 1. Unfortunately, you need to get a committer to approve running the tests.
> I just gave the green-light on your PR.
> 2. You can hope that committers will see your PR, but sometimes things get
> lost. If you know someone who is familiar with that area of the code, it is
> a good idea to ping them.
> 3. We do have some flaky tests. You can see that Jenkins will run 3
> parallel builds, if some of them pass and the committer confirms that
> failures are not related to your code, we are ok to merge. Obviously, if
> you end up tracking them down and fixing, everyone will be very grateful.
>
> Hope this helps,
>
> Gwen
>
> On Sun, Jun 14, 2020 at 5:52 PM Michael Carter <
> michael.carter@instaclustr.com> wrote:
>
>> Hi all,
>>
>> I’ve submitted a patch for the first time(
>> https://github.com/apache/kafka/pull/8844 <
>> https://github.com/apache/kafka/pull/8844>), and I have a couple of
>> questions that I’m hoping someone can help me answer.
>>
>> I’m a little unclear what happens after that patch has been submitted. The
>> coding guidelines say Jenkins will run tests automatically, but I don’t see
>> any results anywhere. Have I misunderstood what should happen, or do I just
>> not know where to look?
>> Should I be attempting to find reviewers for the change myself, or is that
>> done independently of the patch submitter?
>>
>> Also, in resolving a couple of conflicts that have arisen after the patch
>> was first submitted, I noticed that there are now failing unit tests that
>> have nothing to do with my change. Is there a convention on how to deal
>> with these? Should it be something that I try to fix on my branch?
>>
>> Any thoughts are appreciated.
>>
>> Thanks,
>> Michael
>
>
>
> --
> Gwen Shapira
> Engineering Manager | Confluent
> 650.450.2760 | @gwenshap
> Follow us: Twitter | blog
Re: First time patch submitter advice
Posted by Gwen Shapira <gw...@confluent.io>.
Hi,
1. Unfortunately, you need to get a committer to approve running the tests.
I just gave the green-light on your PR.
2. You can hope that committers will see your PR, but sometimes things get
lost. If you know someone who is familiar with that area of the code, it is
a good idea to ping them.
3. We do have some flaky tests. You can see that Jenkins will run 3
parallel builds, if some of them pass and the committer confirms that
failures are not related to your code, we are ok to merge. Obviously, if
you end up tracking them down and fixing, everyone will be very grateful.
Hope this helps,
Gwen
On Sun, Jun 14, 2020 at 5:52 PM Michael Carter <
michael.carter@instaclustr.com> wrote:
> Hi all,
>
> I’ve submitted a patch for the first time(
> https://github.com/apache/kafka/pull/8844 <
> https://github.com/apache/kafka/pull/8844>), and I have a couple of
> questions that I’m hoping someone can help me answer.
>
> I’m a little unclear what happens after that patch has been submitted. The
> coding guidelines say Jenkins will run tests automatically, but I don’t see
> any results anywhere. Have I misunderstood what should happen, or do I just
> not know where to look?
> Should I be attempting to find reviewers for the change myself, or is that
> done independently of the patch submitter?
>
> Also, in resolving a couple of conflicts that have arisen after the patch
> was first submitted, I noticed that there are now failing unit tests that
> have nothing to do with my change. Is there a convention on how to deal
> with these? Should it be something that I try to fix on my branch?
>
> Any thoughts are appreciated.
>
> Thanks,
> Michael
--
Gwen Shapira
Engineering Manager | Confluent
650.450.2760 | @gwenshap
Follow us: Twitter | blog