[GitHub] [incubator-shardingsphere] terrymanu commented on issue #3165: Support time server database for issues 1497

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

terrymanu commented on issue #3165: Support time server database for issues 1497
URL: https://github.com/apache/incubator-shardingsphere/pull/3165#issuecomment-538965476
 
 
   This pull request is too big. I just review something, I found some problems.
   
   1. Should not use `threadlocal` to handle `ShardingConnection`, it will cause the code hard to understand, and may cause multiple threads problems.
   2. Class `MySQLNowExpressionCovertExtractor` is in sharding-jdbc component, how to deal with sharding-proxy if they need use this feature too?
   3. `MySQLNowExpressionCovertExtractor` should be in parse module, not in jdbc module which is for access endpoint only, should not contains any parse and sharding logic.
   4. use `ShardingConnection` in `MySQLNowExpressionCovertExtractor` is unacceptable, because the parse module is a lower layer which should not aware any upper layers, the module dependency is revert here.
   5. I notice this pr add some new SPI in parse layer that is not same layer with original parse SPI, it will make user confuse, any SPI added should fully discuss first.
   
   Because the design and direction of this pr may have some problems, so should I close this pr, and we use mailing-list to talk about design first?
   

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