Ethanlm commented on issue #3223: [STORM-3595] Refactor Resource Aware Strategies (Base, Generic, Default) URL: https://github.com/apache/storm/pull/3223#issuecomment-600314489 It's a good idea to reduce code duplications. But the refactored code seems less extendable and heavily coupled with the parent class. So after the refactoring, the child class needs to set two attributes Attribute 1: sortNodesForEachExecutor Attribute 2: objectResourceSortType Imaging a case here: If we want to implement another child class, which has another requirement, then we need to add an `Attribute 3`, and it will require us to update all the existing classes (BaseResourceAwareStrategy, DefaultResourceAwareStrategy, and GenericResourceAwareStrategy and even the IStrategy interface to reflect the change). For Attribute 2: objectResourceSortType, I am seeing the same code is copied from `DefaultResourceAwareStrategy` and `GenericResourceAwareStrategy` to the parent class `BaseResourceAwareStrategy`, and use `objectResourceSortType` to distinguish between which one of `sortObjectResourcesDefault` and `sortObjectResourcesGeneric` to be used. This doesn't reduce code duplication. But it increased code coupling. For Attribute 1: sortNodesForEachExecutor The duplication mostly happens in whether or not sorting nodes for each executor, in the `schedule` function. Current way of setting a `sortNodesForEachExecutor` variable to control the parent class's `schedule` logic doesn't seem straightforward and increases code coupling. My suggestion on reducing the code duplication is to extract the following part: https://github.com/apache/storm/blob/master/storm-server/src/main/java/org/apache/storm/scheduler/resource/strategies/scheduling/DefaultResourceAwareStrategy.java#L65-L92 from `DefaultResourceAwareStrategy` as a separate function (say **func1**) and re-implement it in `GenericResourceAwareStrategy`. And the hierarchy will be `DefaultResourceAwareStrategy` extends `BaseResourceAwareStrategy`, `GenericResourceAwareStrategy` extends `DefaultResourceAwareStrategy`. We can also have `BaseResourceAwareStrategy` to implement the `schedule` function, and `DefaultResourceAwareStrategy` and `GenericResourceAwareStrategy` both extend `BaseResourceAwareStrategy` and implement **func1** respectively.
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services