[ https://issues.apache.org/jira/browse/YARN-8925?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16678431#comment-16678431 ]
Weiwei Yang edited comment on YARN-8925 at 11/7/18 3:58 PM: ------------------------------------------------------------ Hi [~Tao Yang] Thanks for the patch. It's a nice refactor in {{NodeStatusUpdaterImpl}}, looks pretty good. And also thanks for adding unit tests, the coverage seems good too. Some comments, *NodeLabelUtil#isNodeAttributesEquals* if {{leftNodeAttributes}} is a subset of {{rightNodeAttributes}} seems also equals. And except for the name and value, we also need to compare prefix right? It would be good if we have a separate UT for this method, to verify various of cases. *HeartbeatSyncIfNeededHandler* Can we rename this to {{CachedNodeDescriptorHandler}}? As this class caches the last value of node label/attribute and leverages the cache to reduce the overhead. *TestResourceTrackerService#testNodeRegistrationWithAttributes* {code:java} File tempDir = File.createTempFile("nattr", ".tmp"); {code} can we put tmp dir under {{TEMP_DIR}} that to be consistent with rest of tests. *TestNodeStatusUpdaterForAttributes* waitTillHeartbeat/waitTillHeartbeat can these methods be simplified with GenericTestUtils.waitFor? Thanks was (Author: cheersyang): Hi [~Tao Yang] Thanks for the patch. It's a nice refactor in {{NodeStatusUpdaterImpl}}, looks pretty good. And also thanks for adding unit tests, the coverage seems good too. Some comments, *NodeLabelUtil#isNodeAttributesEquals* if {{leftNodeAttributes}} is a subset of \{{rightNodeAttributes}} seems also equals. And except for the name and value, we also need to compare prefix right? It would be good if we have a separate UT for this method, to verify various of cases. *HeartbeatSyncIfNeededHandler* Can we rename this to \{{CachedNodeDescriptorHandler}}? As this class caches the last value of node label/attribute and leverages the cache to reduce the overhead. *TestResourceTrackerService#testNodeRegistrationWithAttributes* {code} File tempDir = File.createTempFile("nattr", ".tmp"); {code} can we put tmp dir under \{{TEMP_DIR}} that to be consistent with rest of tests. *TestNodeStatusUpdaterForAttributes* waitTillHeartbeat/waitTillHeartbeat can these methods be simplified with GenericTestUtils.waitFor? Thanks > Updating distributed node attributes only when necessary > -------------------------------------------------------- > > Key: YARN-8925 > URL: https://issues.apache.org/jira/browse/YARN-8925 > Project: Hadoop YARN > Issue Type: Sub-task > Components: resourcemanager > Affects Versions: 3.2.1 > Reporter: Tao Yang > Assignee: Tao Yang > Priority: Major > Labels: performance > Attachments: YARN-8925.001.patch, YARN-8925.002.patch, > YARN-8925.003.patch > > > Currently if distributed node attributes exist, even though there is no > change, updating for distributed node attributes will happen in every > heartbeat between NM and RM. Updating process will hold > NodeAttributesManagerImpl#writeLock and may have some influence in a large > cluster. We have found nodes UI of a large cluster is opened slowly and most > time it's waiting for the lock in NodeAttributesManagerImpl. I think this > updating should be called only when necessary to enhance the performance of > related process. -- 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