APEXCORE-10 #resolve Changes for supporting anti-affinity in operators #250
Conversation
ishark
force-pushed
the
APEXCORE-10
branch
2 times, most recently
from
6be49eb to
93fbd6a
Compare
|
@tweise Pleas review |
|
Testing Done:
|
|
@ishark there are merge conflicts, can you please rebase. |
| <artifactId>commons-beanutils-core</artifactId> | ||
| <groupId>commons-beanutils</groupId> | ||
| </exclusion> | ||
| <exclusion> |
There was a problem hiding this comment.
This was added for supporting setting of Affinity Rules from properties.xml. Added a JsonStringCodec for reading Affinity Rule Json..
|
Haven't looked at the plan changes yet, rest looks good so far. |
ishark
force-pushed
the
APEXCORE-10
branch
3 times, most recently
from
2bb1eb2 to
a47829a
Compare
|
Have addressed most of the review comments except convenience APIs in DAG. Did quick sanity testing on Cloudera after changes. |
|
Looking at it. |
| this.setRelaxLocality(relaxLocality); | ||
| } | ||
|
|
||
| public AffinityRule(Type type, Locality locality, boolean relaxLocality, String... operators) |
There was a problem hiding this comment.
Does it require at least 2 operators? Is so, maybe make it String, String...
There was a problem hiding this comment.
Changed it to String, String ..
ishark
force-pushed
the
APEXCORE-10
branch
3 times, most recently
from
e6c27a8 to
dbcbddc
Compare
| List<String> blacklistNodes = resourceRequestor.getNodesExceptHost(requests.get(0).getNodes()); | ||
| amRmClient.updateBlacklist(blacklistNodes, requests.get(0).getNodes()); | ||
| blacklistedNodesForHostSpecificRequests = blacklistNodes; | ||
| LOG.info("Sending {} request(s) after blacklisting nodes {} and removed host from request {}", requests.size(), blacklistNodes, requests.get(0).getNodes()); |
There was a problem hiding this comment.
I still have a concern with the logging of potentially very large collections of nodes (consider cluster size).
ishark
force-pushed
the
APEXCORE-10
branch
3 times, most recently
from
6d7f416 to
32c2ad2
Compare
|
Have addressed review comments and updated diff. |
|
@ishark please resolve the merge conflict |
| ObjectReader reader = mapper.reader(clazz); | ||
| return reader.readValue(string); | ||
| } catch (IOException e) { | ||
| Throwables.propagate(e); |
There was a problem hiding this comment.
Exception propagation. No need for subsequent return null
There was a problem hiding this comment.
Would be nice to fix the other occurrences in this file also and remove usage of DTThrowable.
|
Are the affinity sets updated when containers are added or removed due to dynamic partitioning? If they are not, please create a follow up JIRA so we can address it later. |
…rators Added list of affinity rules as attribute in DAGContext Added validation for affinity, anti-affinity and locality rules in dag validate phase Added handling of Container and Node affinity rules. Rack affinity is not supported at this point Also added Changes for supporting Node specific request in Cloudera Added Unit tests for affinity rules and dag validation for the same Added Json String codec for setting affinity rules through properties.xml Added Set Affinity and anti -affinity APIs to dag Update node reports regularly and keep waiting on requests if host cannot be allocated
|
I have added reassigning of anti-affinity sets for containers in assignContainers after repartitioning is done. But will create a follow up Jira to validate dynamic update scenarios. |
3 participants
Added list of affinity rules as attribute in DAGContext
Added validation for affinity, anti-affinity and locality rules in dag validate phase
Added handling of Container and Node affinity rules. Rack affinity is not supported at this point
Also added Changes for supporting Node specific request in Cloudera
Added Unit tests for affinity rules and dag validation for the same
Added Json String codec for setting affinity rules through properties.xml
Added Set Affinity and anti -affinity APIs to dag