mynameborat commented on a change in pull request #1421:
URL: https://github.com/apache/samza/pull/1421#discussion_r477797973
##########
File path:
samza-core/src/main/java/org/apache/samza/container/LocalityManager.java
##########
@@ -53,28 +56,26 @@ public LocalityManager(MetadataStore metadataStore) {
}
/**
- * Method to allow read container locality information from the {@link
MetadataStore}.
- * This method is used in {@link
org.apache.samza.coordinator.JobModelManager}.
+ * Fetch the container locality information from the {@link MetadataStore}.
*
- * @return the map of containerId: (hostname)
+ * @return the {@code LocalityModel} for the job
*/
- public Map<String, Map<String, String>> readContainerLocality() {
Review comment:
We have divergence in our logic on how lack of locality is handled e.g.
within `LocalityManager` vs `ContainerProcessorManager`. I see your point in
case of complex logic being shared across. However, introducing a helper method
to just unwrap the model seems overkill at the moment.
Also, why only host and what is special about that attribute to have a
helper wrapper method and why not for jmx or jmx tunnel url? This goes back to
my point of being redundant without much ROI at the cost readability because
I'd need to go down one layer further to understand what
`readLastSeenHostForContainer` does in its unwrapping logic.
I'd prefer to start simple and not over architect and extract/refactor as we
see the need as long as the right foundations are in place for the code to
evolve and extend.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
