You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "bharathgunapati (via GitHub)" <gi...@apache.org> on 2026/06/30 19:20:07 UTC
[PR] [FLINK-40030][connector-http] Add architecture tests [flink-connector-http]
bharathgunapati opened a new pull request, #50:
URL: https://github.com/apache/flink-connector-http/pull/50
## What is the purpose of the change
The HTTP connector currently has no architecture tests, unlike most other Flink
connectors (e.g. flink-connector-kafka, flink-connector-jdbc, flink-connector-aws),
which enforce structural rules via the shared flink-architecture-tests framework
(ArchUnit). This PR adds the standard Flink architecture tests to
flink-connector-http so the connector is held to the same architectural guarantees
and new violations are caught automatically in CI. As is conventional when
introducing architecture tests (see FLINK-24138), existing violations are recorded
in a frozen baseline rather than fixed here, keeping the change scoped to adding
the tests.
## Brief change log
- Add test-scoped flink-architecture-tests-test and flink-architecture-tests-production dependencies
- Add ProductionCodeArchitectureTest and TestCodeArchitectureTest under org.apache.flink.architecture
- Add archunit.properties with the standard freezing-store configuration (archRule.failOnEmptyShould=false for rules not applicable to this connector)
- Commit the initial archunit-violations freezing store as a baseline
## Verifying this change
This change is itself a set of architecture tests. It can be verified by running
`mvn -pl flink-connector-http test -Dtest='ProductionCodeArchitectureTest,TestCodeArchitectureTest'`
and confirming both pass against the committed baseline.
## Does this pull request potentially affect one of the following areas?
- **Dependencies**: Yes — adds two test-only dependencies (flink-architecture-tests-test, flink-architecture-tests-production)
- **Performance**: No
- **Security**: No
- **Breaking changes**: No — test-only; no production code or public API is changed
## Documentation
Not applicable — this change adds internal test tooling only and introduces no user-facing feature or configuration.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40030][connector-http] Add architecture tests [flink-connector-http]
Posted by "bharathgunapati (via GitHub)" <gi...@apache.org>.
bharathgunapati commented on PR #50:
URL: https://github.com/apache/flink-connector-http/pull/50#issuecomment-4915347408
@davidradl — as requested, I've opened a follow-up JIRA for the 55 frozen violations in 09015216-6ca6-4513-8e4a-90022b27e585:
[FLINK-40100](https://issues.apache.org/jira/browse/FLINK-40100) — Reduce ArchUnit violations: remove direct usage of Flink-internal/shaded APIs in flink-connector-http
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40030][connector-http] Add architecture tests [flink-connector-http]
Posted by "bharathgunapati (via GitHub)" <gi...@apache.org>.
bharathgunapati commented on PR #50:
URL: https://github.com/apache/flink-connector-http/pull/50#issuecomment-4856334424
Thanks so much for the thorough review, David. Went through all three points:
1. archunit-junit5-engine / silently-skipped tests
I dug into this carefully and I don't think the tests are being skipped — the engine is on the test classpath, transitively via flink-architecture-tests-test → archunit-junit5 → archunit-junit5-engine:
+- com.tngtech.archunit:archunit:1.4.1:test
+- com.tngtech.archunit:archunit-junit5-api:1.4.1:test
\- com.tngtech.archunit:archunit-junit5:1.4.1:test
\- com.tngtech.archunit:archunit-junit5-engine:1.4.1:test
\- com.tngtech.archunit:archunit-junit5-engine-api:1.4.1:test
And the rules do execute — Surefire reports Tests run: 10 for ProductionCodeArchitectureTest and Tests run: 2 for TestCodeArchitectureTest (non-zero counts; a missing engine would give Tests run: 0). To make this un-ambiguous, I've also un-frozen the @VisibleForTesting rule below (see point 3) — the build now goes red on it, which is a direct, in-CI demonstration that the engine is running rather than skipping.
On the suggested swap to the archunit-junit5 bundle: I'd actually lean against it, because it would re-break the maven-dependency-plugin:analyze gate. Our test classes directly reference com.tngtech.archunit.core.importer.ImportOption (from archunit) and the @AnalyzeClasses/@ArchTest annotations (from archunit-junit5-api), and analyze requires the artifacts that contain the used classes to be declared directly. Declaring only the bundle would flag archunit + archunit-junit5-api as "used undeclared" and the bundle as "unused declared." This mirrors what flink-connector-kafka does (declare archunit + archunit-junit5-api, engine transitive), so I've kept it consistent with that.
2. 56 non-public Flink internals
Agreed — freezing is the pragmatic way to land the gate, and these deserve follow-up JIRAs. Most are the shaded-Jackson usage in JavaNetHttpPollingClient, OidcAccessTokenManager, and GenericJsonAndUrlQueryCreator (no public JSON API to migrate to today), plus a few internal table utilities.
3. @VisibleForTesting call from production
Fully agree this one is a genuine smell that should be fixed, not frozen. Small clarification on the earlier state: it wasn't rule-suppressed — the rule always ran; the single violation was just recorded in the freeze baseline. I've now done two things:
Un-froze the rule by removing its entry from the baseline, so NO_CALLS_TO_VISIBLE_FOR_TESTING_METHODS is a hard, enforced gate again (with the annotation still present, the build failed on it — which also confirms the ArchUnit engine is genuinely executing these rules, per point 1).
Applied the fix: LookupRow.getLookupEntries() is a genuine production accessor now (used in the lookup() hot path), so the @VisibleForTesting annotation was simply misleading. I dropped the annotation — it stays package-private in the same package, so no visibility change — and the rule is back to green on the fixed code with an empty baseline (not frozen).
Proof from the CI logs:
<img width="1056" height="574" alt="Screenshot 2026-07-01 at 7 53 04 PM" src="https://github.com/user-attachments/assets/6f6a7396-b67a-4977-bf0e-efeb99fd1adb" />
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40030][connector-http] Add architecture tests [flink-connector-http]
Posted by "bharathgunapati (via GitHub)" <gi...@apache.org>.
bharathgunapati commented on PR #50:
URL: https://github.com/apache/flink-connector-http/pull/50#issuecomment-4847543471
@davidradl when you have a moment, could you please take a look? This adds the standard Flink architecture tests (FLINK-40030) to the HTTP connector, with existing violations frozen as a baseline (same approach as kafka/jdbc). Also, the fork's CI run is awaiting maintainer approval — would appreciate a kick when convenient.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40030][connector-http] Add architecture tests [flink-connector-http]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl merged PR #50:
URL: https://github.com/apache/flink-connector-http/pull/50
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40030][connector-http] Add architecture tests [flink-connector-http]
Posted by "bharathgunapati (via GitHub)" <gi...@apache.org>.
bharathgunapati commented on PR #50:
URL: https://github.com/apache/flink-connector-http/pull/50#issuecomment-4893944293
Hi @davidradl — gentle nudge on this one whenever you have a spare moment.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40030][connector-http] Add architecture tests [flink-connector-http]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on PR #50:
URL: https://github.com/apache/flink-connector-http/pull/50#issuecomment-4855008671
@bharathgunapati I have asked my AI to review this and it came up with :
One blocking issue:
[archunit-junit5-engine](https://github.com/TNG/ArchUnit) is missing from both POMs. Only archunit-junit5-api (annotations) is declared. Without the JUnit Platform engine, all @ArchTest fields are silently skipped — the build goes green because the tests never run, not because they pass. Fix: swap both archunit-junit5-api and archunit for the archunit-junit5 convenience bundle (which includes the engine).
Frozen violations worth noting:
56 violations of using non-public Flink internals — primarily shaded Jackson being used directly in JavaNetHttpPollingClient, OidcAccessTokenManager, and GenericJsonAndUrlQueryCreator. Freezing is correct here to get the gate in, but these warrant follow-up JIRAs.
1 violation where production code calls a @VisibleForTesting method — a genuine code smell that should be fixed, not just frozen.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org