flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [flink] xuefuz edited a comment on issue #8007: [FLINK-11474][table] Add ReadableCatalog, ReadableWritableCatalog, and other …
Date Tue, 26 Mar 2019 20:15:11 GMT
xuefuz edited a comment on issue #8007: [FLINK-11474][table] Add ReadableCatalog, ReadableWritableCatalog,
and other …
URL: https://github.com/apache/flink/pull/8007#issuecomment-476812342
 
 
   > Thanks for the PR, I think catalog is very important for Table/SQL. Great job @xuefuz!
   > 
   > I quickly browsed the PR and left some comments.
   > There are two other suggestions as follows:
   > 
   > * The overall PR does not have a test, it is recommended that  it better to add the
 corresponding test case.
   > * This PR just adds some interfaces, and there is no specific implementation and corresponding
feature. This will confuse reviewer who have no context background, IMHO, each PR preferably
has a specific function, of the function is large, it can be divided into multiple commits
in one PR.
   > 
   > I am not sure whether my suggestion is reasonable or not, maybe @aljoscha and @twalthr
can give us more advice.
   > 
   > What do you think?
   > 
   > Best,
   > Jincheng
   
   Thanks for the review, Jincheng! You have some good points.
   First all, for those missing the contact, please check [FLIP-30.](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=100827366)
   
   Secondly, this one does have a test and the reason is that this is mostly new interfaces,
for which, tests are not really applicable.
   
   Lastly, I generally agree that it's better to complete a feature or functionality with
a single PR. However, for larger efforts such as this one, Incremental commits are preferable
in my opinion because it makes review lighter and easier, especially the incremental code
doesn't break existing stuff.
   

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

Mime
View raw message