[GitHub] [geode] dschneider-pivotal commented on pull request #4692: GEODE-7799: Distribute rebalance status to other locators
done [ Full content available at: https://github.com/apache/geode/pull/4692 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #4692: GEODE-7799: Distribute rebalance status to other locators
We added "getFuture" to the ClusterManagementService. We thought this was cleaner than adding "getFuture" to ClusterManagementOperationResult because if we did that it would need to have a reference to the ClusterManagementService to repeatedly call "get". [ Full content available at: https://github.com/apache/geode/pull/4692 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #4692: GEODE-7799: Distribute rebalance status to other locators
I filed a geode ticket about moving those methods up to OperationResult. The team talked and we decided it would be best to keep the impl class [ Full content available at: https://github.com/apache/geode/pull/4692 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #4692: GEODE-7799: Distribute rebalance status to other locators
I filed: https://issues.apache.org/jira/browse/GEODE-7803 for this [ Full content available at: https://github.com/apache/geode/pull/4692 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #4692: GEODE-7799: Distribute rebalance status to other locators
ps: I'm fine with some other name for the class that implements the RebalanceResult. I also find it odd that RebalanceResult has "getSuccess()" and "getStatusMessage()" but its super interface "OperationResult" is empty. It seems like both of these would be reasonable things to expect from every OperationResult. [ Full content available at: https://github.com/apache/geode/pull/4692 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #4692: GEODE-7799: Distribute rebalance status to other locators
This PR did not introduce RebalanceResult nor RebalanceResultImpl. I prefer keeping the current interface and having an internal implementation class. This prevents our external apis from having internal things exposed. For example, in this case, the external api does not need a constructor nor settors. I think it would be good to have more of our external api contract defined by interfaces and try to minimize the number of classes in it. [ Full content available at: https://github.com/apache/geode/pull/4692 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
