[FLINK-36009] Architecture tests fixes

[eskabetxe]

• Created the package "org.apache.flink.connector.jdbc.core.util" to have classes of flink that are Internal and fails on architecture (could be discussed)
  • Precondition for Preconditions
  • VisibleForTest for VisibleForTesting
  • Other under evaluation, we still have 91 offends on "Connector production code must depend only on public API when outside of connector packages"

[eskabetxe]

@RocMarshal @1996fanrui can you check when have time

[RocMarshal]

I'm curious why it is necessary to create this new class?

I looked at the VisibleForTesting class in Flink and found that it is also annotated as Internal.

Is the reason for introducing this class the same as the purpose of Preconditions?
If so, if we decide to use this approach, we need to modify the relevant configuration items in checkstyle.xml and add the likely comments like the new Preconditions to clarify why need it.
Alternatively, can we initiate a discussion to change the annotations of these classes (VisibleForTesting and Preconditions) in the Flink package to non-internal? (I think there’s probably missed some context in historical discussion about the annotations for related classes from my side. pls correct me if I'm wrong. )

Thanks!

[eskabetxe]

We are duplicating the class because in Flink is annotated with internal and architecture tests fail because of that.. we cannot use any class annotated with internal (this is the same reason for Preconditions and we could have more classes with this problem)

I could revert this changes to allow the discussion, I also think that change on Flink class both classes to non-internal would be great.

[RocMarshal]

Thx for the comment.
Maybe there’s no rush to modify anything before we reaching a conclusion.

I'm not familiar with the entire testing framework and the use of annotations in Flink, so I couldn't make useful ideas.
If we decide to change the relevant annotations and utility classes to non-internal, we should confirm what impact this will have on the main Flink repository. If the impact is minimal, maybe we can initiate a discussion.

Hi, @MartijnVisser could you help give some ideas about it? Many thanks~ :)

[RocMarshal]

If we ultimately keep this class, what problems might arise from using the class name Preconditions?
What I mean is that since the comment mentions this is a code copy, perhaps we can just use the package name to differentiate it and keep the original class-name.

[eskabetxe]

None, I simple give another name to be more easy to find the right class

[RocMarshal]

Hi, @eskabetxe thx for the pr.
LGTM on the whole.
I left a few of comments. PTAL if you had the free time.

[eskabetxe]

@RocMarshal thanks for your review..

I think I could change this to 2 PRs... one with the new architecture module and another with changes to fix the "Connector production code must depend only on public API when outside of connector packages"
What do you think?

[RocMarshal]

@RocMarshal thanks for your review..

I think I could change this to 2 PRs... one with the new architecture module and another with changes to fix the "Connector production code must depend only on public API when outside of connector packages" What do you think?

good idea. Make sense to me~
BTW, The first PR could be done first, and it might be better to start the second task after we have some results from the discussion ?

[eskabetxe]

Created another PR only with new module for architecture change here
This is keeped to allow the discussion about the fixes, and will be simplified so the other PR is merged.