[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16803117#comment-16803117 ] Hudson commented on YARN-9269: -- SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #16295 (See [https://builds.apache.org/job/Hadoop-trunk-Commit/16295/]) YARN-9269. Minor cleanup in FpgaResourceAllocator. Contributed by Peter (devaraj: rev a4cd75e09c934699ec5e2fa969f1c8d0a14c1d49) * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/fpga/FpgaResourceAllocator.java * (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/fpga/FpgaResourceHandlerImpl.java > Minor cleanup in FpgaResourceAllocator > -- > > Key: YARN-9269 > URL: https://issues.apache.org/jira/browse/YARN-9269 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Peter Bacsko >Assignee: Peter Bacsko >Priority: Minor > Fix For: 3.3.0 > > Attachments: YARN-9269-001.patch, YARN-9269-002.patch, > YARN-9269-003.patch, YARN-9269-004.patch, YARN-9269-005.patch > > > Some stuff that we observed: > * {{addFpga()}} - we check for duplicate devices, but we don't print any > error/warning if there's any. > * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is > this method even needed? We already receive an {{FpgaDevice}} instance in > {{updateFpga()}} which I believe is the same that we're looking up. > * variable {{IPIDpreference}} is confusing > * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of > {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple > {{HashMap}} suffice? > * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear > * {{allowedFpgas}} should be an immutable list > * {{@VisibleForTesting}} methods should be package private > * get rid of {{*}} imports -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16802618#comment-16802618 ] Peter Bacsko commented on YARN-9269: [~devaraj.k] [~sunilg] Could you guys review + commit it if it looks good? Thanks! > Minor cleanup in FpgaResourceAllocator > -- > > Key: YARN-9269 > URL: https://issues.apache.org/jira/browse/YARN-9269 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Peter Bacsko >Assignee: Peter Bacsko >Priority: Major > Attachments: YARN-9269-001.patch, YARN-9269-002.patch, > YARN-9269-003.patch, YARN-9269-004.patch, YARN-9269-005.patch > > > Some stuff that we observed: > * {{addFpga()}} - we check for duplicate devices, but we don't print any > error/warning if there's any. > * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is > this method even needed? We already receive an {{FpgaDevice}} instance in > {{updateFpga()}} which I believe is the same that we're looking up. > * variable {{IPIDpreference}} is confusing > * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of > {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple > {{HashMap}} suffice? > * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear > * {{allowedFpgas}} should be an immutable list > * {{@VisibleForTesting}} methods should be package private > * get rid of {{*}} imports -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16801158#comment-16801158 ] Hadoop QA commented on YARN-9269: - | (x) *{color:red}-1 overall{color}* | \\ \\ || Vote || Subsystem || Runtime || Comment || | {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 13s{color} | {color:blue} Docker mode activated. {color} | || || || || {color:brown} Prechecks {color} || | {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s{color} | {color:green} The patch does not contain any @author tags. {color} | | {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s{color} | {color:red} The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. {color} | || || || || {color:brown} trunk Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 16m 33s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 2s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m 25s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 40s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 11m 20s{color} | {color:green} branch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 0m 56s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 23s{color} | {color:green} trunk passed {color} | || || || || {color:brown} Patch Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m 34s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m 58s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m 58s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m 22s{color} | {color:green} hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager: The patch generated 0 new + 27 unchanged - 3 fixed = 27 total (was 30) {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 35s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m 0s{color} | {color:green} The patch has no whitespace issues. {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 11m 30s{color} | {color:green} patch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m 1s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 23s{color} | {color:green} the patch passed {color} | || || || || {color:brown} Other Tests {color} || | {color:green}+1{color} | {color:green} unit {color} | {color:green} 21m 15s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed. {color} | | {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m 29s{color} | {color:green} The patch does not generate ASF License warnings. {color} | | {color:black}{color} | {color:black} {color} | {color:black} 68m 34s{color} | {color:black} {color} | \\ \\ || Subsystem || Report/Notes || | Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f | | JIRA Issue | YARN-9269 | | JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12963657/YARN-9269-005.patch | | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle | | uname | Linux 2660c18d6cae 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | /testptch/patchprocess/precommit/personality/provided.sh | | git revision | trunk / 710cbc9 | | maven | version: Apache Maven 3.3.9 | | Default Java | 1.8.0_191 | | findbugs | v3.1.0-RC1 | | Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/23806/testReport/ | | Max. process+thread count | 447 (vs. ulimit of 1) | | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16801043#comment-16801043 ] Peter Bacsko commented on YARN-9269: Uploaded patch v5, because v4 no longer applies to trunk cleanly. > Minor cleanup in FpgaResourceAllocator > -- > > Key: YARN-9269 > URL: https://issues.apache.org/jira/browse/YARN-9269 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Peter Bacsko >Assignee: Peter Bacsko >Priority: Major > Attachments: YARN-9269-001.patch, YARN-9269-002.patch, > YARN-9269-003.patch, YARN-9269-004.patch, YARN-9269-005.patch > > > Some stuff that we observed: > * {{addFpga()}} - we check for duplicate devices, but we don't print any > error/warning if there's any. > * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is > this method even needed? We already receive an {{FpgaDevice}} instance in > {{updateFpga()}} which I believe is the same that we're looking up. > * variable {{IPIDpreference}} is confusing > * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of > {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple > {{HashMap}} suffice? > * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear > * {{allowedFpgas}} should be an immutable list > * {{@VisibleForTesting}} methods should be package private > * get rid of {{*}} imports -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16800484#comment-16800484 ] Peter Bacsko commented on YARN-9269: [~devaraj.k] could you review this as well? > Minor cleanup in FpgaResourceAllocator > -- > > Key: YARN-9269 > URL: https://issues.apache.org/jira/browse/YARN-9269 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Peter Bacsko >Assignee: Peter Bacsko >Priority: Major > Attachments: YARN-9269-001.patch, YARN-9269-002.patch, > YARN-9269-003.patch, YARN-9269-004.patch > > > Some stuff that we observed: > * {{addFpga()}} - we check for duplicate devices, but we don't print any > error/warning if there's any. > * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is > this method even needed? We already receive an {{FpgaDevice}} instance in > {{updateFpga()}} which I believe is the same that we're looking up. > * variable {{IPIDpreference}} is confusing > * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of > {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple > {{HashMap}} suffice? > * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear > * {{allowedFpgas}} should be an immutable list > * {{@VisibleForTesting}} methods should be package private > * get rid of {{*}} imports -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16800180#comment-16800180 ] Hadoop QA commented on YARN-9269: - | (x) *{color:red}-1 overall{color}* | \\ \\ || Vote || Subsystem || Runtime || Comment || | {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 14s{color} | {color:blue} Docker mode activated. {color} | || || || || {color:brown} Prechecks {color} || | {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s{color} | {color:green} The patch does not contain any @author tags. {color} | | {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s{color} | {color:red} The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. {color} | || || || || {color:brown} trunk Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 16m 29s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 3s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m 29s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 41s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 12m 9s{color} | {color:green} branch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 0m 56s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 27s{color} | {color:green} trunk passed {color} | || || || || {color:brown} Patch Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m 34s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m 58s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m 58s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m 23s{color} | {color:green} hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager: The patch generated 0 new + 34 unchanged - 3 fixed = 34 total (was 37) {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 35s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m 0s{color} | {color:green} The patch has no whitespace issues. {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 11m 59s{color} | {color:green} patch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m 5s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 25s{color} | {color:green} the patch passed {color} | || || || || {color:brown} Other Tests {color} || | {color:green}+1{color} | {color:green} unit {color} | {color:green} 21m 22s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed. {color} | | {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m 28s{color} | {color:green} The patch does not generate ASF License warnings. {color} | | {color:black}{color} | {color:black} {color} | {color:black} 70m 25s{color} | {color:black} {color} | \\ \\ || Subsystem || Report/Notes || | Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f | | JIRA Issue | YARN-9269 | | JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12963561/YARN-9269-004.patch | | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle | | uname | Linux 2b25bca93746 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | /testptch/patchprocess/precommit/personality/provided.sh | | git revision | trunk / 128dd91 | | maven | version: Apache Maven 3.3.9 | | Default Java | 1.8.0_191 | | findbugs | v3.1.0-RC1 | | Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/23796/testReport/ | | Max. process+thread count | 412 (vs. ulimit of 1) | | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16795674#comment-16795674 ] Devaraj K commented on YARN-9269: - Thanks [~pbacsko] for the patch, latest patch is not getting applied, please update it. > Minor cleanup in FpgaResourceAllocator > -- > > Key: YARN-9269 > URL: https://issues.apache.org/jira/browse/YARN-9269 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Peter Bacsko >Assignee: Peter Bacsko >Priority: Major > Attachments: YARN-9269-001.patch, YARN-9269-002.patch, > YARN-9269-003.patch > > > Some stuff that we observed: > * {{addFpga()}} - we check for duplicate devices, but we don't print any > error/warning if there's any. > * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is > this method even needed? We already receive an {{FpgaDevice}} instance in > {{updateFpga()}} which I believe is the same that we're looking up. > * variable {{IPIDpreference}} is confusing > * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of > {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple > {{HashMap}} suffice? > * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear > * {{allowedFpgas}} should be an immutable list > * {{@VisibleForTesting}} methods should be package private > * get rid of {{*}} imports -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16768154#comment-16768154 ] Hadoop QA commented on YARN-9269: - | (x) *{color:red}-1 overall{color}* | \\ \\ || Vote || Subsystem || Runtime || Comment || | {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 23s{color} | {color:blue} Docker mode activated. {color} | || || || || {color:brown} Prechecks {color} || | {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s{color} | {color:green} The patch does not contain any @author tags. {color} | | {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s{color} | {color:red} The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. {color} | || || || || {color:brown} trunk Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 18m 17s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 30s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m 33s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 47s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 13m 45s{color} | {color:green} branch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m 14s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 33s{color} | {color:green} trunk passed {color} | || || || || {color:brown} Patch Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m 42s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 18s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m 18s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m 29s{color} | {color:green} hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager: The patch generated 0 new + 50 unchanged - 8 fixed = 50 total (was 58) {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 44s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m 0s{color} | {color:green} The patch has no whitespace issues. {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 14m 21s{color} | {color:green} patch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m 18s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 29s{color} | {color:green} the patch passed {color} | || || || || {color:brown} Other Tests {color} || | {color:green}+1{color} | {color:green} unit {color} | {color:green} 22m 13s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed. {color} | | {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m 28s{color} | {color:green} The patch does not generate ASF License warnings. {color} | | {color:black}{color} | {color:black} {color} | {color:black} 79m 9s{color} | {color:black} {color} | \\ \\ || Subsystem || Report/Notes || | Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f | | JIRA Issue | YARN-9269 | | JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12958705/YARN-9269-003.patch | | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle | | uname | Linux 0c5593d0a8da 4.4.0-138-generic #164~14.04.1-Ubuntu SMP Fri Oct 5 08:56:16 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | /testptch/patchprocess/precommit/personality/provided.sh | | git revision | trunk / dfe0f42 | | maven | version: Apache Maven 3.3.9 | | Default Java | 1.8.0_191 | | findbugs | v3.1.0-RC1 | | Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/23402/testReport/ | | Max. process+thread count | 305 (vs. ulimit of 1) | | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hado
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16768129#comment-16768129 ] Adam Antal commented on YARN-9269: -- Perfect, +1 (non-binding). > Minor cleanup in FpgaResourceAllocator > -- > > Key: YARN-9269 > URL: https://issues.apache.org/jira/browse/YARN-9269 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Peter Bacsko >Assignee: Peter Bacsko >Priority: Major > Attachments: YARN-9269-001.patch, YARN-9269-002.patch, > YARN-9269-003.patch > > > Some stuff that we observed: > * {{addFpga()}} - we check for duplicate devices, but we don't print any > error/warning if there's any. > * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is > this method even needed? We already receive an {{FpgaDevice}} instance in > {{updateFpga()}} which I believe is the same that we're looking up. > * variable {{IPIDpreference}} is confusing > * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of > {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple > {{HashMap}} suffice? > * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear > * {{allowedFpgas}} should be an immutable list > * {{@VisibleForTesting}} methods should be package private > * get rid of {{*}} imports -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16762983#comment-16762983 ] Adam Antal commented on YARN-9269: -- Thanks you for the answers, I agree. Sorry for missing YARN-9268, I'll take a look at that as well soon. Maybe allowedFpgas could be renamed to reflect that it is a kind of initializer (so it is called only once), but it's ok how this is now. Pending new patch and jenkins, but the patch looks good to me. > Minor cleanup in FpgaResourceAllocator > -- > > Key: YARN-9269 > URL: https://issues.apache.org/jira/browse/YARN-9269 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Peter Bacsko >Assignee: Peter Bacsko >Priority: Major > Attachments: YARN-9269-001.patch, YARN-9269-002.patch > > > Some stuff that we observed: > * {{addFpga()}} - we check for duplicate devices, but we don't print any > error/warning if there's any. > * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is > this method even needed? We already receive an {{FpgaDevice}} instance in > {{updateFpga()}} which I believe is the same that we're looking up. > * variable {{IPIDpreference}} is confusing > * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of > {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple > {{HashMap}} suffice? > * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear > * {{allowedFpgas}} should be an immutable list > * {{@VisibleForTesting}} methods should be package private > * get rid of {{*}} imports -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16762946#comment-16762946 ] Peter Bacsko commented on YARN-9269: Thanks [~adam.antal] for the comments. 1. {{allowedFpgas}} is populated only once and never modified again. It's called from the {{bootstrap()}} method when the NM starts up. So I add the elements to a local list then wrap it in an immutable list. 2. Yeah but that technically cannot happen. Also, we no longer retrieve the device object from a list - we have it already as a method argument. Anyway I'll have to double check that this is still OK (I haven't tested any modifications on a cluster yet). 3. Unused stuff in FpgaDevice: yeah, there's a separate Jira for that: YARN-9268. We store stuff which is unnecessary. Only aliasDevName, minor and major are important. 4. Ok, I will do the rename + remove the extra spaces. > Minor cleanup in FpgaResourceAllocator > -- > > Key: YARN-9269 > URL: https://issues.apache.org/jira/browse/YARN-9269 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Peter Bacsko >Assignee: Peter Bacsko >Priority: Major > Attachments: YARN-9269-001.patch, YARN-9269-002.patch > > > Some stuff that we observed: > * {{addFpga()}} - we check for duplicate devices, but we don't print any > error/warning if there's any. > * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is > this method even needed? We already receive an {{FpgaDevice}} instance in > {{updateFpga()}} which I believe is the same that we're looking up. > * variable {{IPIDpreference}} is confusing > * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of > {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple > {{HashMap}} suffice? > * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear > * {{allowedFpgas}} should be an immutable list > * {{@VisibleForTesting}} methods should be package private > * get rid of {{*}} imports -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16762899#comment-16762899 ] Adam Antal commented on YARN-9269: -- Thanks for the patch, [~pbacsko]. Nice job, the quality of the code is much better now. Here are some thoughts of mine: - Could you please rationale why has the behaviour of addFpga changed? Previously the devices from the variable list (also there can be some more descriptive name of it) is processed this way: devices are being added one by one to allowedFpgas list, if they're not already added. Now we collect them into a list named fpgaDevices, and at the end of the function we overwrite the list allowedFpgas, so the devices added to the allowedFpgas list before the function is called can be lost. - I agree that the function findMatchedFpga is not needed (basically just a type of search/"indexOf" function), but the updateFpga function's side effect is that it also checks whether usedFpgaByRequestor (renamed to containerToFpgaMapping) contains that device or not, and if not contained a warning is raised. Even though this function is only called from FpgaResourceHandlerImpl:preStart, where we know that this case does not happen, I think we should explicitly keep that. - There are some unused function in FpgaDevice class: setDevName, setAliasDevName, setBusNum. It makes sense to have getters and setters for all the class variables, but they're also missing from temperature, and card power usage, so I think the above mentioned function could be removed. - It's really a minor thing, but can we rename the class variable availableFpga to availableFpgas (so an extra s, implying its more than one device). - There are also useless extra spaces on the top of the file, right after the license and after the package statement. Please enlight me, if I don't see something clear. > Minor cleanup in FpgaResourceAllocator > -- > > Key: YARN-9269 > URL: https://issues.apache.org/jira/browse/YARN-9269 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Peter Bacsko >Assignee: Peter Bacsko >Priority: Major > Attachments: YARN-9269-001.patch, YARN-9269-002.patch > > > Some stuff that we observed: > * {{addFpga()}} - we check for duplicate devices, but we don't print any > error/warning if there's any. > * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is > this method even needed? We already receive an {{FpgaDevice}} instance in > {{updateFpga()}} which I believe is the same that we're looking up. > * variable {{IPIDpreference}} is confusing > * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of > {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple > {{HashMap}} suffice? > * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear > * {{allowedFpgas}} should be an immutable list > * {{@VisibleForTesting}} methods should be package private > * get rid of {{*}} imports -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16761762#comment-16761762 ] Hadoop QA commented on YARN-9269: - | (x) *{color:red}-1 overall{color}* | \\ \\ || Vote || Subsystem || Runtime || Comment || | {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 31s{color} | {color:blue} Docker mode activated. {color} | || || || || {color:brown} Prechecks {color} || | {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s{color} | {color:green} The patch does not contain any @author tags. {color} | | {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s{color} | {color:red} The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. {color} | || || || || {color:brown} trunk Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 24m 0s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 22s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m 33s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 45s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 13m 48s{color} | {color:green} branch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m 18s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 29s{color} | {color:green} trunk passed {color} | || || || || {color:brown} Patch Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m 43s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 13s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m 13s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m 26s{color} | {color:green} hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager: The patch generated 0 new + 21 unchanged - 8 fixed = 21 total (was 29) {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 44s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m 0s{color} | {color:green} The patch has no whitespace issues. {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 14m 18s{color} | {color:green} patch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m 30s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 30s{color} | {color:green} the patch passed {color} | || || || || {color:brown} Other Tests {color} || | {color:green}+1{color} | {color:green} unit {color} | {color:green} 22m 7s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed. {color} | | {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m 53s{color} | {color:green} The patch does not generate ASF License warnings. {color} | | {color:black}{color} | {color:black} {color} | {color:black} 85m 21s{color} | {color:black} {color} | \\ \\ || Subsystem || Report/Notes || | Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f | | JIRA Issue | YARN-9269 | | JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12957753/YARN-9269-002.patch | | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle | | uname | Linux 173a6c9a8cae 4.4.0-138-generic #164~14.04.1-Ubuntu SMP Fri Oct 5 08:56:16 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | /testptch/patchprocess/precommit/personality/provided.sh | | git revision | trunk / 911790c | | maven | version: Apache Maven 3.3.9 | | Default Java | 1.8.0_191 | | findbugs | v3.1.0-RC1 | | Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/23313/testReport/ | | Max. process+thread count | 338 (vs. ulimit of 1) | | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hado
[jira] [Commented] (YARN-9269) Minor cleanup in FpgaResourceAllocator
[ https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16761695#comment-16761695 ] Hadoop QA commented on YARN-9269: - | (x) *{color:red}-1 overall{color}* | \\ \\ || Vote || Subsystem || Runtime || Comment || | {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 13s{color} | {color:blue} Docker mode activated. {color} | || || || || {color:brown} Prechecks {color} || | {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 1s{color} | {color:green} The patch does not contain any @author tags. {color} | | {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s{color} | {color:red} The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. {color} | || || || || {color:brown} trunk Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 23m 39s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 5s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m 29s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 43s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 12m 18s{color} | {color:green} branch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m 3s{color} | {color:green} trunk passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 27s{color} | {color:green} trunk passed {color} | || || || || {color:brown} Patch Compile Tests {color} || | {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m 36s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 0s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m 0s{color} | {color:green} the patch passed {color} | | {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange} 0m 22s{color} | {color:orange} hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager: The patch generated 4 new + 22 unchanged - 7 fixed = 26 total (was 29) {color} | | {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 37s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m 0s{color} | {color:green} The patch has no whitespace issues. {color} | | {color:green}+1{color} | {color:green} shadedclient {color} | {color:green} 12m 30s{color} | {color:green} patch has no errors when building and testing our client artifacts. {color} | | {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m 8s{color} | {color:green} the patch passed {color} | | {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 22s{color} | {color:green} the patch passed {color} | || || || || {color:brown} Other Tests {color} || | {color:green}+1{color} | {color:green} unit {color} | {color:green} 21m 12s{color} | {color:green} hadoop-yarn-server-nodemanager in the patch passed. {color} | | {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m 28s{color} | {color:green} The patch does not generate ASF License warnings. {color} | | {color:black}{color} | {color:black} {color} | {color:black} 78m 3s{color} | {color:black} {color} | \\ \\ || Subsystem || Report/Notes || | Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f | | JIRA Issue | YARN-9269 | | JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12957743/YARN-9269-001.patch | | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle | | uname | Linux f3d9f1ce031c 4.4.0-139-generic #165~14.04.1-Ubuntu SMP Wed Oct 31 10:55:11 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | /testptch/patchprocess/precommit/personality/provided.sh | | git revision | trunk / 911790c | | maven | version: Apache Maven 3.3.9 | | Default Java | 1.8.0_191 | | findbugs | v3.1.0-RC1 | | checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/23312/artifact/out/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt | | Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/23312/testReport/ | | Max. process+threa