libenchao commented on PR #14376: URL: https://github.com/apache/flink/pull/14376#issuecomment-1159021778
@maosuhan I started to review this PR today. Apologies for the delay. I didn't go deep into the code yet, but there are a few general issues which we'd better handle them in the first place. 1. About packaging, we'd better add a 'flink-sql-protobuf' module to build a bundle jar for pure sql usages 2. I noticed you introduced scala dependency for this module, hence we'd better to add the scala version to the artifactId, you may find some examples here: https://github.com/apache/flink/blob/3ae4c6f5a48105d00807e8ce02e70d4c092cbf40/flink-streaming-scala/pom.xml#L30 3. Flink community is migrating to [Junit5](https://issues.apache.org/jira/browse/FLINK-25325), we better to keep align with that (I'm also fine to do it in another following separate issue) 4. About documentation, of course we need it, especially for an official release. Since we are targeting to 1.16, so the documentation should also be prepared in 1.16 (It can also be done in another following separate issue IMO) 5. About the classloading, since we need users to provide the compiled pb classes, and load them dynamically, we'd better to load them using context classloader, you can find some examples here: https://github.com/apache/flink/blob/3ae4c6f5a48105d00807e8ce02e70d4c092cbf40/flink-connectors/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/internal/connection/SimpleJdbcConnectionProvider.java#L90 6. Since 1.15 has been released, master's version is not 1.16-SNAPSHOT, you'd better rebase your pr the latest master, and change the version. (I did this locally, and found some API changes which leads to compiling errors) -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
