Thanks for the review comments Vinod Kumar Vavilapalli,
SCRIPT_NODE_LABELS_PROVIDER and CONFIG_NODE_LABELS_PROVIDER are not needed, delete them, you have separate constants for their prefixes
Actually these are not preffixes, as per Tan, Wangda's comment we had decided have whitelisting for provider : The option will be: yarn.node-labels.nm.provider = "config/script/other-class-name". . These are modifications for it.
DISABLE_NODE_LABELS_PROVIDER_FETCH_TIMER doesn't need to be in YarnConfiguration
Well As per one of the wangda's comment he had mentioned that possible values or default values of configurations had to be kept in YARNConfiguration, hence had placed it here, if required as per your comment can move it to AbstractNodeLabelsProvider
LOG is not used anywhere
Are the logs expected when the labels are set in setNodeLabels ? i can add here but anyway there were logs in NodeStatusUpdaterImpl on successfull and unsuccessfull attempts.
YARN-3565 goes in first, you will have to make some changes here.
I think the format expected from the command should be more structured. Specifically as we expect more per-label attributes in line with
Well was thinking about this while working on
YARN-3565, but dint modify the NodeLabelsProvider as currently Labels(currently partitions) which needs to be sent from NM have to be one of RM's CLUSTER NodeLabel set. So exclusiveness need not be sent from NM to RM as currently we support specifying the exclusiveness only during adding clusterNode labels. So IMHO if there is plan to make this interface public & stable then would be better do these changes now itself if not it would better done after requirement for constraint labels, so that more clarity on structure would be there?
Tan, Wangda and you can share your opinion on this, based on it will do the modifications.
Not caused by your patch but worth fixing here. NodeStatusUpdaterImpl shouldn't worry about invalid label-set, previous-valid-labels and label validation. You should move all that functionality into NodeLabelsProvider.
Well as per the class reponsibility i understand that NodeStatusUpdaterImpl is not supposed to have it but as it might be expected to be public we had to ensure that
- For every heartbeat labels are sent across only if modified
- doing basic validations before sending the modified labels
These needs to be done irrespective of the label provider (system or user's) hence kept it in NodeStatusUpdaterImpl , but if req to be moved out then we need to bring in some intermediate manager(/helper/delegator) class between NodeStatusUpdaterImpl and NodeLabelsProvider.
Those changes were also from my previous patch, so no hard feelings in taking care of it if req .
Can you add the documentation for setting this up too too?
Well was planning to raise jira for updating documentation on top of NodeLabels but documentation for it is not yet completed. If required can just add some pdf here