[GitHub] [storm] Ethanlm edited a comment on issue #3223: [STORM-3595] Refactor Resource Aware Strategies (Base, Generic, Default)

[GitBox <gi...@apache.org>]

Ethanlm edited a comment 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. 
   
   After this 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:
users@infra.apache.org


With regards,
Apache Git Services