You are viewing a plain text version of this content. The canonical link for it is here.
Posted to mapreduce-dev@hadoop.apache.org by Sean Busbey <sb...@apple.com.INVALID> on 2021/05/13 15:10:02 UTC
[DISCUSS] check style changes
Hi folks!
I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
—
busbey
---------------------------------------------------------------------
To unsubscribe, e-mail: mapreduce-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: mapreduce-dev-help@hadoop.apache.org
Re: [DISCUSS] check style changes
Posted by Ayush Saxena <ay...@gmail.com>.
If you tend to change the line limit, It should have a proper DISCUSS and VOTE thread. If I remember it correct for ozone it was discussed to increase the limit to 100 from 80 and that got vetoed.
So, we too should get an agreement first. Well I have gone through the ozone discussion and I also don’t support changing the line length limit in general.
Regarding the nightly runs, I think fixing the flaky tests and getting away with the frequent OOM errors in the build is something should get priority, that would be helpfull to the project.
Fixing checkstyles, firstly would be just a look and feel change. Many checkstyle warnings would be the one accepted by the committer while commiting and the most important stuff, it would make backports tough, checking the git history a little tougher, and apart from that these changes would be huge. So reviewers need to be extra cautious, that accidentally some bug doesn’t get induced.
So, Personal opinions: Line length stuff needs proper discussion and vote, justifying a strong reason.
Chekstyle modifications also, if it is like tooo much of change, and no big advantages, I think atleast I can survive with it, giving the fact it might take my ease to backport issues internally as well as to other branches. But still since it won’t break anything, so I don’t think so, I have any right to say No to this. But in case you plan to pick this up with priority. Get the plan discussed before merging.
Would be happy to help in case you plan to pick up the other stuffs related to the builds. :-)
-Ayush
> On 13-May-2021, at 8:40 PM, Sean Busbey <sb...@apple.com.invalid> wrote:
>
> Hi folks!
>
> I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
>
> As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
>
> If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
>
> —
> busbey
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: common-dev-unsubscribe@hadoop.apache.org
> For additional commands, e-mail: common-dev-help@hadoop.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: common-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-dev-help@hadoop.apache.org
Re: [DISCUSS] check style changes
Posted by "epayne@apache.org" <ep...@apache.org>.
I would be fine with a discussion and vote on relaxing some checkstyle restrictions.
Regarding line length, my personal preference is to leave it at 80, but 80 is arbitrary and I would not oppose 100 if that's what people want.
Another one that I think should be relaxed is the limit on number of arguments to a method. I understand that a ton of arguments makes a method messy, but I find it irritating when I add an argument to something that is already over the limit and I get penalized for it. The ones I have seen are all constructor methods.
-Eric
On Thursday, May 13, 2021, 10:10:27 AM CDT, Sean Busbey <sb...@apple.com.invalid> wrote:
Hi folks!
I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
—
busbey
---------------------------------------------------------------------
To unsubscribe, e-mail: mapreduce-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: mapreduce-dev-help@hadoop.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-dev-help@hadoop.apache.org
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Vinayakumar B <vi...@apache.org>.
Call.toString() was used only while sending the response, in the response
processor thread. At this point, header would be already read and
processed. So no chance of race conditions.
This race condition was not introduced by the HADOOP-17046
<https://issues.apache.org/jira/browse/HADOOP-17046>, instead existing code
was never intended to be used for logging purposes by multiple threads.
Though the locking mechanism proposed in HADOOP-18143
<https://issues.apache.org/jira/browse/HADOOP-18143> solves the problems,
it adds overhead in both memory as well as performance.
This is a critical path for RPC processing, and adding a
locking/synchronization in this place would affect the overall performance
(may not be huge).
I would advise to remove logging and locking both altogether and leave the
code as is in Reader thread.
If logging is required, then add it in the response processor thread or the
handler threads.
-Vinay
On Mon, Feb 28, 2022 at 10:02 PM Ayush Saxena <ay...@gmail.com> wrote:
> Hey Andras,
> I had a quick look at HADOOP-18143, the methods in question in
> ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
> not very sure how the race condition doesn't happen in ProtobufRpcEngine.
> I have to debug and spend some more time, considering that I have
> reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
> be there as you said, but will give us some time to analyse.
>
> Thanks
> -Ayush
>
> On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
> wrote:
>
>> Hey everyone!
>>
>> We have started seeing test failures in YARN PRs for a while. We have
>> identified the problematic commit, which is HADOOP-18082
>> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this
>> change
>> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
>> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We
>> have
>> also fixed the underlying issue via a locking mechanism, presented in
>> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
>> since it is out of our area of expertise, we can neither verify nor
>> guarantee that it will not cause some subtle issues in the RPC system.
>> As we think it is a core part of Hadoop, we would use feedback from
>> someone
>> who is proficient in this part.
>>
>> Regards:
>> Andras
>>
>
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Vinayakumar B <vi...@apache.org>.
Call.toString() was used only while sending the response, in the response
processor thread. At this point, header would be already read and
processed. So no chance of race conditions.
This race condition was not introduced by the HADOOP-17046
<https://issues.apache.org/jira/browse/HADOOP-17046>, instead existing code
was never intended to be used for logging purposes by multiple threads.
Though the locking mechanism proposed in HADOOP-18143
<https://issues.apache.org/jira/browse/HADOOP-18143> solves the problems,
it adds overhead in both memory as well as performance.
This is a critical path for RPC processing, and adding a
locking/synchronization in this place would affect the overall performance
(may not be huge).
I would advise to remove logging and locking both altogether and leave the
code as is in Reader thread.
If logging is required, then add it in the response processor thread or the
handler threads.
-Vinay
On Mon, Feb 28, 2022 at 10:02 PM Ayush Saxena <ay...@gmail.com> wrote:
> Hey Andras,
> I had a quick look at HADOOP-18143, the methods in question in
> ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
> not very sure how the race condition doesn't happen in ProtobufRpcEngine.
> I have to debug and spend some more time, considering that I have
> reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
> be there as you said, but will give us some time to analyse.
>
> Thanks
> -Ayush
>
> On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
> wrote:
>
>> Hey everyone!
>>
>> We have started seeing test failures in YARN PRs for a while. We have
>> identified the problematic commit, which is HADOOP-18082
>> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this
>> change
>> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
>> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We
>> have
>> also fixed the underlying issue via a locking mechanism, presented in
>> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
>> since it is out of our area of expertise, we can neither verify nor
>> guarantee that it will not cause some subtle issues in the RPC system.
>> As we think it is a core part of Hadoop, we would use feedback from
>> someone
>> who is proficient in this part.
>>
>> Regards:
>> Andras
>>
>
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Vinayakumar B <vi...@apache.org>.
Call.toString() was used only while sending the response, in the response
processor thread. At this point, header would be already read and
processed. So no chance of race conditions.
This race condition was not introduced by the HADOOP-17046
<https://issues.apache.org/jira/browse/HADOOP-17046>, instead existing code
was never intended to be used for logging purposes by multiple threads.
Though the locking mechanism proposed in HADOOP-18143
<https://issues.apache.org/jira/browse/HADOOP-18143> solves the problems,
it adds overhead in both memory as well as performance.
This is a critical path for RPC processing, and adding a
locking/synchronization in this place would affect the overall performance
(may not be huge).
I would advise to remove logging and locking both altogether and leave the
code as is in Reader thread.
If logging is required, then add it in the response processor thread or the
handler threads.
-Vinay
On Mon, Feb 28, 2022 at 10:02 PM Ayush Saxena <ay...@gmail.com> wrote:
> Hey Andras,
> I had a quick look at HADOOP-18143, the methods in question in
> ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
> not very sure how the race condition doesn't happen in ProtobufRpcEngine.
> I have to debug and spend some more time, considering that I have
> reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
> be there as you said, but will give us some time to analyse.
>
> Thanks
> -Ayush
>
> On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
> wrote:
>
>> Hey everyone!
>>
>> We have started seeing test failures in YARN PRs for a while. We have
>> identified the problematic commit, which is HADOOP-18082
>> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this
>> change
>> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
>> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We
>> have
>> also fixed the underlying issue via a locking mechanism, presented in
>> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
>> since it is out of our area of expertise, we can neither verify nor
>> guarantee that it will not cause some subtle issues in the RPC system.
>> As we think it is a core part of Hadoop, we would use feedback from
>> someone
>> who is proficient in this part.
>>
>> Regards:
>> Andras
>>
>
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Vinayakumar B <vi...@apache.org>.
Call.toString() was used only while sending the response, in the response
processor thread. At this point, header would be already read and
processed. So no chance of race conditions.
This race condition was not introduced by the HADOOP-17046
<https://issues.apache.org/jira/browse/HADOOP-17046>, instead existing code
was never intended to be used for logging purposes by multiple threads.
Though the locking mechanism proposed in HADOOP-18143
<https://issues.apache.org/jira/browse/HADOOP-18143> solves the problems,
it adds overhead in both memory as well as performance.
This is a critical path for RPC processing, and adding a
locking/synchronization in this place would affect the overall performance
(may not be huge).
I would advise to remove logging and locking both altogether and leave the
code as is in Reader thread.
If logging is required, then add it in the response processor thread or the
handler threads.
-Vinay
On Mon, Feb 28, 2022 at 10:02 PM Ayush Saxena <ay...@gmail.com> wrote:
> Hey Andras,
> I had a quick look at HADOOP-18143, the methods in question in
> ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
> not very sure how the race condition doesn't happen in ProtobufRpcEngine.
> I have to debug and spend some more time, considering that I have
> reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
> be there as you said, but will give us some time to analyse.
>
> Thanks
> -Ayush
>
> On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
> wrote:
>
>> Hey everyone!
>>
>> We have started seeing test failures in YARN PRs for a while. We have
>> identified the problematic commit, which is HADOOP-18082
>> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this
>> change
>> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
>> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We
>> have
>> also fixed the underlying issue via a locking mechanism, presented in
>> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
>> since it is out of our area of expertise, we can neither verify nor
>> guarantee that it will not cause some subtle issues in the RPC system.
>> As we think it is a core part of Hadoop, we would use feedback from
>> someone
>> who is proficient in this part.
>>
>> Regards:
>> Andras
>>
>
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Vinayakumar B <vi...@apache.org>.
Call.toString() was used only while sending the response, in the response
processor thread. At this point, header would be already read and
processed. So no chance of race conditions.
This race condition was not introduced by the HADOOP-17046
<https://issues.apache.org/jira/browse/HADOOP-17046>, instead existing code
was never intended to be used for logging purposes by multiple threads.
Though the locking mechanism proposed in HADOOP-18143
<https://issues.apache.org/jira/browse/HADOOP-18143> solves the problems,
it adds overhead in both memory as well as performance.
This is a critical path for RPC processing, and adding a
locking/synchronization in this place would affect the overall performance
(may not be huge).
I would advise to remove logging and locking both altogether and leave the
code as is in Reader thread.
If logging is required, then add it in the response processor thread or the
handler threads.
-Vinay
On Mon, Feb 28, 2022 at 10:02 PM Ayush Saxena <ay...@gmail.com> wrote:
> Hey Andras,
> I had a quick look at HADOOP-18143, the methods in question in
> ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
> not very sure how the race condition doesn't happen in ProtobufRpcEngine.
> I have to debug and spend some more time, considering that I have
> reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
> be there as you said, but will give us some time to analyse.
>
> Thanks
> -Ayush
>
> On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
> wrote:
>
>> Hey everyone!
>>
>> We have started seeing test failures in YARN PRs for a while. We have
>> identified the problematic commit, which is HADOOP-18082
>> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this
>> change
>> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
>> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We
>> have
>> also fixed the underlying issue via a locking mechanism, presented in
>> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
>> since it is out of our area of expertise, we can neither verify nor
>> guarantee that it will not cause some subtle issues in the RPC system.
>> As we think it is a core part of Hadoop, we would use feedback from
>> someone
>> who is proficient in this part.
>>
>> Regards:
>> Andras
>>
>
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Ayush Saxena <ay...@gmail.com>.
Hey Andras,
I had a quick look at HADOOP-18143, the methods in question in
ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
not very sure how the race condition doesn't happen in ProtobufRpcEngine.
I have to debug and spend some more time, considering that I have
reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
be there as you said, but will give us some time to analyse.
Thanks
-Ayush
On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
wrote:
> Hey everyone!
>
> We have started seeing test failures in YARN PRs for a while. We have
> identified the problematic commit, which is HADOOP-18082
> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this change
> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We have
> also fixed the underlying issue via a locking mechanism, presented in
> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
> since it is out of our area of expertise, we can neither verify nor
> guarantee that it will not cause some subtle issues in the RPC system.
> As we think it is a core part of Hadoop, we would use feedback from someone
> who is proficient in this part.
>
> Regards:
> Andras
>
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Ayush Saxena <ay...@gmail.com>.
Hey Andras,
I had a quick look at HADOOP-18143, the methods in question in
ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
not very sure how the race condition doesn't happen in ProtobufRpcEngine.
I have to debug and spend some more time, considering that I have
reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
be there as you said, but will give us some time to analyse.
Thanks
-Ayush
On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
wrote:
> Hey everyone!
>
> We have started seeing test failures in YARN PRs for a while. We have
> identified the problematic commit, which is HADOOP-18082
> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this change
> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We have
> also fixed the underlying issue via a locking mechanism, presented in
> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
> since it is out of our area of expertise, we can neither verify nor
> guarantee that it will not cause some subtle issues in the RPC system.
> As we think it is a core part of Hadoop, we would use feedback from someone
> who is proficient in this part.
>
> Regards:
> Andras
>
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Ayush Saxena <ay...@gmail.com>.
Hey Andras,
I had a quick look at HADOOP-18143, the methods in question in
ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
not very sure how the race condition doesn't happen in ProtobufRpcEngine.
I have to debug and spend some more time, considering that I have
reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
be there as you said, but will give us some time to analyse.
Thanks
-Ayush
On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
wrote:
> Hey everyone!
>
> We have started seeing test failures in YARN PRs for a while. We have
> identified the problematic commit, which is HADOOP-18082
> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this change
> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We have
> also fixed the underlying issue via a locking mechanism, presented in
> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
> since it is out of our area of expertise, we can neither verify nor
> guarantee that it will not cause some subtle issues in the RPC system.
> As we think it is a core part of Hadoop, we would use feedback from someone
> who is proficient in this part.
>
> Regards:
> Andras
>
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Ayush Saxena <ay...@gmail.com>.
Hey Andras,
I had a quick look at HADOOP-18143, the methods in question in
ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
not very sure how the race condition doesn't happen in ProtobufRpcEngine.
I have to debug and spend some more time, considering that I have
reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
be there as you said, but will give us some time to analyse.
Thanks
-Ayush
On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
wrote:
> Hey everyone!
>
> We have started seeing test failures in YARN PRs for a while. We have
> identified the problematic commit, which is HADOOP-18082
> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this change
> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We have
> also fixed the underlying issue via a locking mechanism, presented in
> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
> since it is out of our area of expertise, we can neither verify nor
> guarantee that it will not cause some subtle issues in the RPC system.
> As we think it is a core part of Hadoop, we would use feedback from someone
> who is proficient in this part.
>
> Regards:
> Andras
>
Re: [DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Ayush Saxena <ay...@gmail.com>.
Hey Andras,
I had a quick look at HADOOP-18143, the methods in question in
ProtobufRpcEngine2 are identical to the ones in ProtobufRpcEngine. So, I am
not very sure how the race condition doesn't happen in ProtobufRpcEngine.
I have to debug and spend some more time, considering that I have
reverted HADOOP-18082 for now to unblock YARN. Though the issue would still
be there as you said, but will give us some time to analyse.
Thanks
-Ayush
On Mon, 28 Feb 2022 at 21:26, Gyori Andras <ga...@cloudera.com.invalid>
wrote:
> Hey everyone!
>
> We have started seeing test failures in YARN PRs for a while. We have
> identified the problematic commit, which is HADOOP-18082
> <https://issues.apache.org/jira/browse/HADOOP-18082>, however, this change
> just revealed the race condition lying in ProtobufRpcEngine2 introduced in
> HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We have
> also fixed the underlying issue via a locking mechanism, presented in
> HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
> since it is out of our area of expertise, we can neither verify nor
> guarantee that it will not cause some subtle issues in the RPC system.
> As we think it is a core part of Hadoop, we would use feedback from someone
> who is proficient in this part.
>
> Regards:
> Andras
>
[DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Gyori Andras <ga...@cloudera.com.INVALID>.
Hey everyone!
We have started seeing test failures in YARN PRs for a while. We have
identified the problematic commit, which is HADOOP-18082
<https://issues.apache.org/jira/browse/HADOOP-18082>, however, this change
just revealed the race condition lying in ProtobufRpcEngine2 introduced in
HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We have
also fixed the underlying issue via a locking mechanism, presented in
HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
since it is out of our area of expertise, we can neither verify nor
guarantee that it will not cause some subtle issues in the RPC system.
As we think it is a core part of Hadoop, we would use feedback from someone
who is proficient in this part.
Regards:
Andras
Re: [DISCUSS] check style changes
Posted by Ayush Saxena <ay...@gmail.com>.
If you tend to change the line limit, It should have a proper DISCUSS and VOTE thread. If I remember it correct for ozone it was discussed to increase the limit to 100 from 80 and that got vetoed.
So, we too should get an agreement first. Well I have gone through the ozone discussion and I also don’t support changing the line length limit in general.
Regarding the nightly runs, I think fixing the flaky tests and getting away with the frequent OOM errors in the build is something should get priority, that would be helpfull to the project.
Fixing checkstyles, firstly would be just a look and feel change. Many checkstyle warnings would be the one accepted by the committer while commiting and the most important stuff, it would make backports tough, checking the git history a little tougher, and apart from that these changes would be huge. So reviewers need to be extra cautious, that accidentally some bug doesn’t get induced.
So, Personal opinions: Line length stuff needs proper discussion and vote, justifying a strong reason.
Chekstyle modifications also, if it is like tooo much of change, and no big advantages, I think atleast I can survive with it, giving the fact it might take my ease to backport issues internally as well as to other branches. But still since it won’t break anything, so I don’t think so, I have any right to say No to this. But in case you plan to pick this up with priority. Get the plan discussed before merging.
Would be happy to help in case you plan to pick up the other stuffs related to the builds. :-)
-Ayush
> On 13-May-2021, at 8:40 PM, Sean Busbey <sb...@apple.com.invalid> wrote:
>
> Hi folks!
>
> I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
>
> As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
>
> If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
>
> —
> busbey
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: common-dev-unsubscribe@hadoop.apache.org
> For additional commands, e-mail: common-dev-help@hadoop.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: mapreduce-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: mapreduce-dev-help@hadoop.apache.org
Re: [DISCUSS] check style changes
Posted by "epayne@apache.org" <ep...@apache.org>.
I would be fine with a discussion and vote on relaxing some checkstyle restrictions.
Regarding line length, my personal preference is to leave it at 80, but 80 is arbitrary and I would not oppose 100 if that's what people want.
Another one that I think should be relaxed is the limit on number of arguments to a method. I understand that a ton of arguments makes a method messy, but I find it irritating when I add an argument to something that is already over the limit and I get penalized for it. The ones I have seen are all constructor methods.
-Eric
On Thursday, May 13, 2021, 10:10:27 AM CDT, Sean Busbey <sb...@apple.com.invalid> wrote:
Hi folks!
I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
—
busbey
---------------------------------------------------------------------
To unsubscribe, e-mail: mapreduce-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: mapreduce-dev-help@hadoop.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-dev-help@hadoop.apache.org
[DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Gyori Andras <ga...@cloudera.com.INVALID>.
Hey everyone!
We have started seeing test failures in YARN PRs for a while. We have
identified the problematic commit, which is HADOOP-18082
<https://issues.apache.org/jira/browse/HADOOP-18082>, however, this change
just revealed the race condition lying in ProtobufRpcEngine2 introduced in
HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We have
also fixed the underlying issue via a locking mechanism, presented in
HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
since it is out of our area of expertise, we can neither verify nor
guarantee that it will not cause some subtle issues in the RPC system.
As we think it is a core part of Hadoop, we would use feedback from someone
who is proficient in this part.
Regards:
Andras
Re: [DISCUSS] check style changes
Posted by Ayush Saxena <ay...@gmail.com>.
If you tend to change the line limit, It should have a proper DISCUSS and VOTE thread. If I remember it correct for ozone it was discussed to increase the limit to 100 from 80 and that got vetoed.
So, we too should get an agreement first. Well I have gone through the ozone discussion and I also don’t support changing the line length limit in general.
Regarding the nightly runs, I think fixing the flaky tests and getting away with the frequent OOM errors in the build is something should get priority, that would be helpfull to the project.
Fixing checkstyles, firstly would be just a look and feel change. Many checkstyle warnings would be the one accepted by the committer while commiting and the most important stuff, it would make backports tough, checking the git history a little tougher, and apart from that these changes would be huge. So reviewers need to be extra cautious, that accidentally some bug doesn’t get induced.
So, Personal opinions: Line length stuff needs proper discussion and vote, justifying a strong reason.
Chekstyle modifications also, if it is like tooo much of change, and no big advantages, I think atleast I can survive with it, giving the fact it might take my ease to backport issues internally as well as to other branches. But still since it won’t break anything, so I don’t think so, I have any right to say No to this. But in case you plan to pick this up with priority. Get the plan discussed before merging.
Would be happy to help in case you plan to pick up the other stuffs related to the builds. :-)
-Ayush
> On 13-May-2021, at 8:40 PM, Sean Busbey <sb...@apple.com.invalid> wrote:
>
> Hi folks!
>
> I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
>
> As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
>
> If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
>
> —
> busbey
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: common-dev-unsubscribe@hadoop.apache.org
> For additional commands, e-mail: common-dev-help@hadoop.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: dev-help@hadoop.apache.org
Re: [DISCUSS] check style changes
Posted by "epayne@apache.org" <ep...@apache.org>.
I would be fine with a discussion and vote on relaxing some checkstyle restrictions.
Regarding line length, my personal preference is to leave it at 80, but 80 is arbitrary and I would not oppose 100 if that's what people want.
Another one that I think should be relaxed is the limit on number of arguments to a method. I understand that a ton of arguments makes a method messy, but I find it irritating when I add an argument to something that is already over the limit and I get penalized for it. The ones I have seen are all constructor methods.
-Eric
On Thursday, May 13, 2021, 10:10:27 AM CDT, Sean Busbey <sb...@apple.com.invalid> wrote:
Hi folks!
I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
—
busbey
---------------------------------------------------------------------
To unsubscribe, e-mail: mapreduce-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: mapreduce-dev-help@hadoop.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-dev-help@hadoop.apache.org
[DISCUSS] Race condition in ProtobufRpcEngine2
Posted by Gyori Andras <ga...@cloudera.com.INVALID>.
Hey everyone!
We have started seeing test failures in YARN PRs for a while. We have
identified the problematic commit, which is HADOOP-18082
<https://issues.apache.org/jira/browse/HADOOP-18082>, however, this change
just revealed the race condition lying in ProtobufRpcEngine2 introduced in
HADOOP-17046 <https://issues.apache.org/jira/browse/HADOOP-17046>. We have
also fixed the underlying issue via a locking mechanism, presented in
HADOOP-18143 <https://issues.apache.org/jira/browse/HADOOP-18143>, but
since it is out of our area of expertise, we can neither verify nor
guarantee that it will not cause some subtle issues in the RPC system.
As we think it is a core part of Hadoop, we would use feedback from someone
who is proficient in this part.
Regards:
Andras
Re: [DISCUSS] check style changes
Posted by Ayush Saxena <ay...@gmail.com>.
If you tend to change the line limit, It should have a proper DISCUSS and VOTE thread. If I remember it correct for ozone it was discussed to increase the limit to 100 from 80 and that got vetoed.
So, we too should get an agreement first. Well I have gone through the ozone discussion and I also don’t support changing the line length limit in general.
Regarding the nightly runs, I think fixing the flaky tests and getting away with the frequent OOM errors in the build is something should get priority, that would be helpfull to the project.
Fixing checkstyles, firstly would be just a look and feel change. Many checkstyle warnings would be the one accepted by the committer while commiting and the most important stuff, it would make backports tough, checking the git history a little tougher, and apart from that these changes would be huge. So reviewers need to be extra cautious, that accidentally some bug doesn’t get induced.
So, Personal opinions: Line length stuff needs proper discussion and vote, justifying a strong reason.
Chekstyle modifications also, if it is like tooo much of change, and no big advantages, I think atleast I can survive with it, giving the fact it might take my ease to backport issues internally as well as to other branches. But still since it won’t break anything, so I don’t think so, I have any right to say No to this. But in case you plan to pick this up with priority. Get the plan discussed before merging.
Would be happy to help in case you plan to pick up the other stuffs related to the builds. :-)
-Ayush
> On 13-May-2021, at 8:40 PM, Sean Busbey <sb...@apple.com.invalid> wrote:
>
> Hi folks!
>
> I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
>
> As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
>
> If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
>
> —
> busbey
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: common-dev-unsubscribe@hadoop.apache.org
> For additional commands, e-mail: common-dev-help@hadoop.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-dev-help@hadoop.apache.org
Re: [DISCUSS] check style changes
Posted by Ayush Saxena <ay...@gmail.com>.
If you tend to change the line limit, It should have a proper DISCUSS and VOTE thread. If I remember it correct for ozone it was discussed to increase the limit to 100 from 80 and that got vetoed.
So, we too should get an agreement first. Well I have gone through the ozone discussion and I also don’t support changing the line length limit in general.
Regarding the nightly runs, I think fixing the flaky tests and getting away with the frequent OOM errors in the build is something should get priority, that would be helpfull to the project.
Fixing checkstyles, firstly would be just a look and feel change. Many checkstyle warnings would be the one accepted by the committer while commiting and the most important stuff, it would make backports tough, checking the git history a little tougher, and apart from that these changes would be huge. So reviewers need to be extra cautious, that accidentally some bug doesn’t get induced.
So, Personal opinions: Line length stuff needs proper discussion and vote, justifying a strong reason.
Chekstyle modifications also, if it is like tooo much of change, and no big advantages, I think atleast I can survive with it, giving the fact it might take my ease to backport issues internally as well as to other branches. But still since it won’t break anything, so I don’t think so, I have any right to say No to this. But in case you plan to pick this up with priority. Get the plan discussed before merging.
Would be happy to help in case you plan to pick up the other stuffs related to the builds. :-)
-Ayush
> On 13-May-2021, at 8:40 PM, Sean Busbey <sb...@apple.com.invalid> wrote:
>
> Hi folks!
>
> I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
>
> As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
>
> If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
>
> —
> busbey
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: common-dev-unsubscribe@hadoop.apache.org
> For additional commands, e-mail: common-dev-help@hadoop.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-dev-help@hadoop.apache.org
Re: [DISCUSS] check style changes
Posted by "epayne@apache.org" <ep...@apache.org>.
I would be fine with a discussion and vote on relaxing some checkstyle restrictions.
Regarding line length, my personal preference is to leave it at 80, but 80 is arbitrary and I would not oppose 100 if that's what people want.
Another one that I think should be relaxed is the limit on number of arguments to a method. I understand that a ton of arguments makes a method messy, but I find it irritating when I add an argument to something that is already over the limit and I get penalized for it. The ones I have seen are all constructor methods.
-Eric
On Thursday, May 13, 2021, 10:10:27 AM CDT, Sean Busbey <sb...@apple.com.invalid> wrote:
Hi folks!
I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
—
busbey
---------------------------------------------------------------------
To unsubscribe, e-mail: mapreduce-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: mapreduce-dev-help@hadoop.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: mapreduce-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: mapreduce-dev-help@hadoop.apache.org
Re: [DISCUSS] check style changes
Posted by "epayne@apache.org" <ep...@apache.org>.
I would be fine with a discussion and vote on relaxing some checkstyle restrictions.
Regarding line length, my personal preference is to leave it at 80, but 80 is arbitrary and I would not oppose 100 if that's what people want.
Another one that I think should be relaxed is the limit on number of arguments to a method. I understand that a ton of arguments makes a method messy, but I find it irritating when I add an argument to something that is already over the limit and I get penalized for it. The ones I have seen are all constructor methods.
-Eric
On Thursday, May 13, 2021, 10:10:27 AM CDT, Sean Busbey <sb...@apple.com.invalid> wrote:
Hi folks!
I’d like to start cleaning up our nightly tests. As a bit of low hanging fruit I’d like to alter some of our check style rules to match what I think we’ve been doing in the community. How would folks prefer I make sure we have consensus on such changes?
As an example, our last nightly run had ~81k check style violations (it’s a big number but it’s not that bad given the size of the repo) and roughly 16% of those were for line lengths in excess of 80 characters but <= 100 characters.
If I wanted to change our line length check to be 100 characters rather than the default of 80, would folks rather I have a DISCUSS thread first? Or would they rather a Jira + PR with the discussion of the merits happening there?
—
busbey
---------------------------------------------------------------------
To unsubscribe, e-mail: mapreduce-dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: mapreduce-dev-help@hadoop.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hadoop.apache.org
For additional commands, e-mail: dev-help@hadoop.apache.org