IGNITE-28799 Fix documentation code snippets compilation#13266
Conversation
chesnokoff
force-pushed
the
ignite-28799-snippets
branch
from
e383626 to
0bec290
Compare
|
Run to check compilation Also try to run checks on #13257 before merge |
| <dependency> | ||
| <groupId>${project.groupId}</groupId> | ||
| <artifactId>ignite-cloud</artifactId> | ||
| <version>2.13.0</version> |
There was a problem hiding this comment.
is it from extensions ? if so - why ver is hardcoded ?
There was a problem hiding this comment.
Yes, ignite-cloud was moved from the main Ignite repository to ignite-extensions as ignite-cloud-ext, but it looks like ignite-cloud-ext was never published to Maven Central. As a result, the only resolvable artifact containing TcpDiscoveryCloudIpFinder is the legacy ignite-cloud:2.13.0.
There was a problem hiding this comment.
There was a problem hiding this comment.
Started discussion on dev list https://lists.apache.org/thread/d214wbsp84gm6sc93woctf3q68dhrsch
anton-vinogradov
left a comment
anton-vinogradov
left a comment
There was a problem hiding this comment.
Solid and genuinely needed cleanup. The published snippets currently teach code that does not compile — wrong class names (TcpDiscoveryAzureBlobStorageIpFinder), an Azure var assigned new TcpDiscoveryGoogleStorageIpFinder(), missing semicolons in JavaThinClient, undefined ccfg/part/PART_CUSTOM_AFFINITY_CACHE_NAME, a wrong package on EchoService*, JUnit 4 @Test against a JUnit 5 module — and they even leak internal API (U.resolveWorkDirectory, F.asMap) into user-facing docs. Replacing those with public API and fixing the CacheInterceptor/generics signatures is the right call.
I checked the rendering side: all 95 snippet include::s use indent=0, so wrapping the former free-floating WarmUpStrategy statements into a class + methods does not change the rendered output. Good.
Three things to settle before merge:
1. This fix is throwaway without the CI gate, and the two PRs aren't formally coupled. IGNITE-28800 (#13257) adds the compile check and edits the same pom.xml. Without it the snippets rot again exactly as they did here, so the value of this PR decays in weeks. Please land them as a coordinated pair and pick the order explicitly: merging the gate first turns master red (this fix isn't in yet); merging this first leaves an unguarded window. Safest is to rebase #13257 on top of this, verify green, and merge back-to-back (resolving the inevitable pom.xml conflict).
2. We're putting the docs build on external ignite-extensions artifacts, one of which doesn't exist. ignite-cloud-ext is not on Maven Central, hence the legacy ignite-cloud:2.13.0 fallback — good that you opened the dev-list thread. Beyond that one: the gate's red/green now depends on Maven Central availability and on *-ext:1.0.0 (released independently of core) staying API-compatible with 2.19.0-SNAPSHOT. Worth confirming ignite-cloud:2.13.0 actually compiles against 2.19 core, and deciding whether the cloud-discovery snippets should be in scope for the gate at all — otherwise a third-party (un)publish can turn core CI red.
3. The acceptance criterion isn't demonstrated yet. The ticket is literally "snippets compile", but there's no green TC visa / successful mvn -f docs/_docs/code-snippets/java/pom.xml compile attached, and the Copilot pass bailed on quota. Please attach the green run (ideally via #13257's workflow) before merge.
Nice-to-have while you're in these files: memory-configuration/data-regions.adoc prose says "pass LoadAllWarmUpStrategy/NoOpWarmUpStrategy to setDefaultWarmUpConfiguration", but the now-compiling snippet passes LoadAllWarmUpConfiguration/NoOpWarmUpConfiguration — the prose names the strategy, the API takes the configuration.
Inline notes below.
anton-vinogradov
left a comment
anton-vinogradov
left a comment
There was a problem hiding this comment.
Switching to request changes to track the two merge-readiness blockers from my detailed review above. This is not about the snippet fixes themselves — those are correct and a real improvement (they currently teach non-compiling code and internal API). Purely about readiness:
-
Acceptance criterion isn't demonstrated. The ticket is literally "snippets compile", but there's no green CI / TC visa and no attached
mvn -f docs/_docs/code-snippets/java/pom.xml compile -B -Voutput (the Copilot pass bailed on quota). Please attach a green run before merge. -
Coordinate the merge with the CI gate (IGNITE-28800 / #13257). Both PRs edit the same
docs/_docs/code-snippets/java/pom.xml, and this fix is throwaway without the gate — the snippets rotted precisely because nothing compiled them. Please land them as a coordinated pair with an explicit order (rebase #13257 on top of this, verify green, merge back-to-back) somasternever goes red and the snippets can't silently rot again.
Still open (not blocking on its own): the ignite-cloud:2.13.0 legacy fallback, since ignite-cloud-ext isn't published — fine to proceed once the dev-list thread concludes, but please confirm 2.13.0 actually compiles against ignite-core:2.19.0-SNAPSHOT.
Happy to flip to approve once the green run is attached and the merge order with #13257 is settled.
|
Confirmed with |
|
All actionable code comments should be addressed now For the CI gate: the intended merge order is to land this snippets-fix PR first, then update #13257 on top of it and merge the CI gate immediately after it is green. Merging #13257 first would make master red because the snippets fixes are not there yet |
anton-vinogradov
left a comment
anton-vinogradov
left a comment
There was a problem hiding this comment.
Verified the acceptance criterion locally — it's green.
Ran the canonical check from the ticket on the PR head against ignite-core:2.19.0-SNAPSHOT, with the extension artifacts resolved from Maven Central:
./mvnw -f docs/_docs/code-snippets/java/pom.xml -DskipTests compile -B -V
...
[INFO] Compiling 82 source files with javac [debug target 17] to target/classes
[INFO] BUILD SUCCESS
Only deprecation/unchecked warnings, no errors. This also settles my earlier open question: ignite-cloud:2.13.0 does compile against ignite-core:2.19.0-SNAPSHOT. So blocker #1 is satisfiable — please attach an equivalent green run (ideally through #13257's workflow) to the ticket.
I re-checked the substance and it holds up: the Azure rename is correct (azure-ext really ships TcpDiscoveryAzureBlobStoreIpFinder, there is no …BlobStorageIpFinder); opencensus-exporter-trace-zipkin:0.31.1 is genuinely required by Tracing.java and matches core's opencensus.version; and every added import is actually used.
One consistency miss — flagging here because the file isn't in the diff. EchoService.java and EchoServiceImpl.java were moved to org.apache.ignite.snippets.services, but their sibling in the same directory, EchoServiceAccessInterceptor.java, still declares package org.apache.ignite.examples;. It compiles (javac tolerates the package/dir mismatch) and the stale line is outside the rendered service-call-interceptor tag, so it doesn't reach the docs — but it's the same source/doc drift this ticket is about, and the class ends up under org/apache/ignite/examples/. One-liner:
// EchoServiceAccessInterceptor.java
package org.apache.ignite.snippets.services;Remaining gating item: the merge order with the CI gate (#13257) on the shared pom.xml — without the gate the snippets rot again. Happy to flip to approve once that's settled and a green run is attached. Optional pom hardening inline below.
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.