[FLINK-36009] Architecture tests refactor #143
Conversation
eskabetxe
force-pushed
the
FLINK-35363_archtectureonly
branch
from
9e44d50 to
02c3783
Compare
There was a problem hiding this comment.
Hi, @eskabetxe Thank you for your hard work.
LGTM on the whole just the commit message header.
Would you mind helping update it ? thx~
eskabetxe
changed the title
fixed, the incorrect one was the PR that was using the Epic |
There was a problem hiding this comment.
Thanks @eskabetxe for the update
LGTM +1 after merging & rebasing #144
|
@1996fanrui can you check |
There was a problem hiding this comment.
@1996fanrui can you check
This PR is related to Architecture refactor, since I'm not an expert of jdbc-connector, so it's better to involve a commiter or PMC member related to connector, thanks.
cc @leonardBang
eskabetxe
force-pushed
the
FLINK-35363_archtectureonly
branch
from
02c3783 to
99a0e50
Compare
|
@leonardBang @MartijnVisser can you help here.. |
| import org.apache.flink.table.types.logical.RowType; | ||
|
|
||
| /** JDBC converter for PostgreSQL compatible databases. */ | ||
| @PublicEvolving | ||
| @Internal |
There was a problem hiding this comment.
this is unclear to me
shouldn't we create a kind of FLIP first if we make such changes?
There was a problem hiding this comment.
This configuration is currently misaligned. All dialect implementations are classified as internal. The purpose of this class is solely to allow the CrateDB dialect to utilize the PostgreSQL implementation.
To preserve the PublicEvolving status, we need to add an exception to the architecture test. Without this exception, the rule "Public Evolving API Methods Use Only Public Evolving API Types" will fail.
To resolve this issue without an exception, we would need to refactor all dialect implementations and classify them as PublicEvolving.
I can add the exception if you still believe that a FLIP is necessary and focus the discussion solely on that change. I believe this will not impact the rest of the PR.
There was a problem hiding this comment.
I can add the exception if you still believe that a FLIP is necessary and focus the discussion solely on that change. I believe this will not impact the rest of the PR.
would be great.
Agree, rest of the PR is more or less ok (I did only a brief look, will look again a bit later)
There was a problem hiding this comment.
@snuyanzin already changed and added the exception to architecture tests.
When you have time could you check again..
Thanks
eskabetxe
force-pushed
the
FLINK-35363_archtectureonly
branch
from
99a0e50 to
6d49ebe
Compare
Created the module flink-connector-jdbc-architecture to have all tests, we need to add the modules as dependency to validate that module..