[GitHub] [geode] albertogpz commented on pull request #6052: GEODE-8971: Add grace period when stopping gateway sender with group-…

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

albertogpz commented on pull request #6052:
URL: https://github.com/apache/geode/pull/6052#issuecomment-787864365


   > I've requested a few changes in-line and a couple general comments that aren't requests for changes.
   > 
   > A good rule is to always avoid raw types (`Predicate condition`) by providing a type (`Predicate<Object> condition`).
   > 
   > I really want to see us moving away from passing around impl classes like `GatewaySenderEventImpl` if an interface like `GatewayQueueEvent` defines the methods you need to call. If you need to call an impl method that's NOT on the interface, then use the impl for now. We'll probably eventually create a new internal interface like `InternalGatewayQueueEvent` if we find that we need to pass around the impl to call non-interface-defined methods. This will help us avoid concrete dependencies which then helps produce higher quality code and unit tests.
   > 
   > Looks like the existing `throw` statements for `BucketRegionQueueUnavailableException` all have no message. It's always better to provide a message, but it's ok as is since it's at least consistent with the other lines that throw it.
   
   
   
   > I've requested a few changes in-line and a couple general comments that aren't requests for changes.
   > 
   > A good rule is to always avoid raw types (`Predicate condition`) by providing a type (`Predicate<Object> condition`).
   > 
   > I really want to see us moving away from passing around impl classes like `GatewaySenderEventImpl` if an interface like `GatewayQueueEvent` defines the methods you need to call. If you need to call an impl method that's NOT on the interface, then use the impl for now. We'll probably eventually create a new internal interface like `InternalGatewayQueueEvent` if we find that we need to pass around the impl to call non-interface-defined methods. This will help us avoid concrete dependencies which then helps produce higher quality code and unit tests.
   > 
   > Looks like the existing `throw` statements for `BucketRegionQueueUnavailableException` all have no message. It's always better to provide a message, but it's ok as is since it's at least consistent with the other lines that throw it.
   
   I have added the isLastEventInTransaction() and getTransactionId() to the GatewayQueueEvent interface. Do you see any drawback to it?
   


----------------------------------------------------------------
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