[
https://issues.apache.org/jira/browse/IGNITE-29261?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Aleksandr Chesnokov updated IGNITE-29261:
-----------------------------------------
Description:
Review specification, 12 November 2026.
* Ticket: https://issues.apache.org/jira/browse/IGNITE-29261
* PR: https://github.com/apache/ignite/pull/13802
* Reviewed branch: {{ignite-29261}}, head
{{47ffdfebf2b5da486a8eb6d372ca798d5da59deb}}.
* Comparison base for this review: {{master}},
{{b0c397908d0b336459620bb1865f790597aae800}}.
This document records the agreed scope and later decisions for this ticket. It
gives a reviewer the context without the earlier chat. The follow-up work below
is a separate task, not a requirement for this PR.
h2. Problem and goal
Ignite generates JSON descriptions of message fields, IDTO classes, enums and
message IDs. Today, contributors must update and commit these files. A
contributor may only learn about this requirement after CI runs near the end of
review.
We want CI to generate the descriptions and show the changes before merge,
without an extra JSON commit. A JSON diff means that the wire format or
registrations need review. It does not prove that compatibility is broken.
Compatibility tests and manual review remain necessary.
h2. Scope of this ticket
# Remove generated JSON files from {{modules/wire-compatibility-check/wire/}}
in Git, including {{ids.json}}.
# Generate and collect the files under
{{modules/wire-compatibility-check/target/classes/META-INF/ignite-wire/}}.
# Remove the CI check that requires generated JSON changes to be committed.
# Compare descriptions from two builds during the normal PR checks and publish
their diff in GitHub Actions Job Summary.
# Keep the existing checks for missing message descriptions and descriptions
not collected by the module.
# Add short local generation instructions in
{{modules/wire-compatibility-check/DEVNOTES.md}}.
The JSON schema, message ID assignment and serialization code do not change in
this ticket.
h2. When the comparison runs
The comparison is a separate job, {{check-wire}}, named {{Compare wire format
descriptions}}, in the existing {{.github/workflows/commit-check.yml}}. It runs
together with the other PR checks. It does not depend on the result of
{{check-java}}. Use the default GitHub Actions timeout; the earlier custom
30-minute limit was removed.
Use the existing {{pull_request}} trigger and repository rules for approving
external contributor runs. No separate manual action is required to run this
comparison. The job is skipped for {{push}} events because they do not provide
the PR context. The normal checks should be run early enough for the reviewer
to read the diff before final approval.
There is no target-branch name filter. PRs to {{master}} and release branches
use the same rule. This extends the original ticket description, which
mentioned only {{master}}.
h2. Which revisions are compared
Compare the target branch snapshot with the result of a test merge of the PR:
||Side||Revision||
| Base | {{github.event.pull_request.base.sha}} |
| PR merge | {{github.sha}} for the {{pull_request}} event |
The default {{actions/checkout}} behavior for a PR checks out this merge
revision. Build the two revisions in separate directories. Record both SHAs in
the Summary.
This shows what merging the PR would change relative to its target branch at
the time of the event. For example, if the target branch already contains a
wire change absent from the PR head, that change is present on both sides and
is not reported as a change made by the PR.
Do not compare the target branch directly with the PR head. Do not use
merge-base versus PR head: that was an earlier option, and the agreed choice is
now target versus PR merge. A report describes its recorded revisions; a later
target-branch update does not make that report a check of the new target tip.
h2. Generation and collection
Use Java 17 and the Maven wrapper. For each revision, build
{{wire-compatibility-check}} and its required modules:
{code:bash}
./mvnw -B -ntp clean test-compile -pl modules/wire-compatibility-check -am -T 1C
{code}
Use {{test-compile}} because the reactor needs test classes. It does not
execute the tests, so CI does not need {{-DskipTests}}. Keep {{clean}} to avoid
files left by an earlier build. Delete the old
{{modules/wire-compatibility-check/wire/}} directory before each build as well,
so the comparison cannot use JSON left in Git by an older revision.
New revisions collect JSON and write {{ids.json}} under
{{target/classes/META-INF/ignite-wire/}} in the module. {{ids.json}} maps
message IDs to message classes and must be included in the directory comparison.
The base of this PR still generates files into the old {{wire/}} directory.
Support both layouts: use the new directory when it contains {{ids.json}};
otherwise use {{wire/}}. These must be freshly generated files. The separate
shell check that failed explicitly on missing {{ids.json}} was removed by
agreement. The remaining file test selects the directory; it is not a general
validation of old generators.
Keep the existing {{MessageIdsWriter}} validation for registered messages
without descriptions. Keep {{Check all wire format descriptions are collected}}
in {{check-java}}, updated to check the new collection directory. It still
fails when a built module produces a description that was not collected. Do not
replace these checks with warnings.
h2. Results shown to the reviewer
Compare the complete directories, including added and removed files and
{{ids.json}}.
||Result||Expected behavior||
| No differences | The job succeeds. Summary says {{No wire format changes.}} |
| Differences found | The job succeeds with a warning. Summary shows the diff
preview. |
| Build, collection or comparison error | The relevant job fails. An error must
not be reported as an empty diff. |
Show at most the first 200,000 bytes of the diff in Summary. When there are
differences, upload the complete {{wire.diff}} as the {{wire-diff}} artifact
with 14-day retention. The preview must point the reviewer to the full artifact.
The presence of a diff is informational and does not fail the job. Errors from
the build or from {{diff}} itself must still fail it. Use read-only repository
permissions and do not persist checkout credentials.
h2. Local use and limits
{{modules/wire-compatibility-check/DEVNOTES.md}} must explain why the JSON
files exist, give a runnable command from the project root, state the output
directory and tell contributors not to commit the generated files. The root
{{DEVNOTES.txt}} stays unchanged.
Old release branches may lack the module, a generator or message ID generation.
Removing the target-branch filter does not add these features to old branches.
Support for each such branch needs a separate check; this ticket does not
promise full wire or ID comparison for all historical releases.
The check compares generated descriptions. It is not a semantic compatibility
analyzer and cannot guarantee detection of every change in hand-written
serialization. A PR with merge conflicts may have no {{pull_request}}
test-merge run; this ticket does not add another trigger for that case.
h2. Acceptance scenarios
* A contributor changes a message and pushes only source changes. CI generates
the JSON; no JSON update commit is required.
* A wire change produces a warning, a Summary preview with both SHAs and a full
diff artifact. A change that leaves JSON unchanged produces the no-change
message.
* A message registration change appears in {{ids.json}} in the diff.
* A PR whose branch is behind its target is compared as target versus PR merge,
so changes already in the target are not reported as PR changes.
* A PR to another target branch uses that branch's SHA. Validate this with a
branch that has the required generators.
* Both the old {{wire/}} layout and the new {{target/}} layout are generated
from scratch and can be compared during this transition.
* A build failure, an uncollected description or a technical comparison error
produces a failed check, not a successful no-change report.
* The local command and output path are documented. Generated JSON remains
outside Git.
h2. Follow-up: compatibility label through workflow_run
Label changes are deliberately outside IGNITE-29261. This PR does not change
{{check-protected-classes.yml}}, publish bot comments, add write permissions or
change the PR description.
The next task should replace the current Java-text rule for the
{{compatibility}} label with the JSON comparison result. After a successful
check of the current PR head, add the label when there is a diff and remove it
when there is no diff. A failed or cancelled check, or a result for an older PR
head, must not change the label.
The intended event chain is:
# The existing {{pull_request}} job builds, compares and publishes its result
as data in an artifact. A successful no-change result must also be available to
the label workflow; absence of a diff artifact alone is not proof of success.
# A separate {{workflow_run}} workflow with {{types: [completed]}} reads that
result, checks that it belongs to the current open PR head and updates the
label. Its code comes from the default branch. It must not check out, build or
execute PR code or artifact contents.
ASF documents the replacement of {{pull_request_target}} with this split
between a read-only build and a trusted workflow with write permissions. The
new workflow must exist on the default branch before the event chain can be
tested. The old label writer must be retired as part of that task so that two
rules do not control the same label.
Anton's earlier proposal used {{workflow_run: requested}} to set a label from
source-file changes before the build. Our later agreement uses the JSON result,
so it needs {{completed}}. Neither version requires moving the Maven comparison
itself to {{workflow_run}}.
Bot comments, automatic JSON commits, a semantic compatibility analyzer and
generator backports to old release branches are not part of this ticket. A
semantic analyzer remains a possible later step, not an agreed implementation
requirement.
h2. References
* [Broader v2 proposal on the Ignite dev
list|https://lists.apache.org/thread.html/hq3d8opxscww09pobvqfvwgc1d0v9cls].
The decisions above record the later choices and the smaller scope of this
ticket.
* [ASF GitHub Actions
policy|https://infra.apache.org/github-actions-policy.html].
* [GitHub workflow
events|https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows],
including {{pull_request}} and {{workflow_run}}.
was:Let's remove the generated wire format JSON files from Git, so
contributors do not need to update and commit them. For PRs to master, CI
should generate the descriptions for two versions and show their diff in the
GitHub Actions Job Summary for review.
> Generate and compare wire format descriptions in CI instead of tracking them
> in Git
> -----------------------------------------------------------------------------------
>
> Key: IGNITE-29261
> URL: https://issues.apache.org/jira/browse/IGNITE-29261
> Project: Ignite
> Issue Type: Improvement
> Reporter: Aleksandr Chesnokov
> Assignee: Aleksandr Chesnokov
> Priority: Major
> Fix For: 2.20
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Review specification, 12 November 2026.
> * Ticket: https://issues.apache.org/jira/browse/IGNITE-29261
> * PR: https://github.com/apache/ignite/pull/13802
> * Reviewed branch: {{ignite-29261}}, head
> {{47ffdfebf2b5da486a8eb6d372ca798d5da59deb}}.
> * Comparison base for this review: {{master}},
> {{b0c397908d0b336459620bb1865f790597aae800}}.
> This document records the agreed scope and later decisions for this ticket.
> It gives a reviewer the context without the earlier chat. The follow-up work
> below is a separate task, not a requirement for this PR.
> h2. Problem and goal
> Ignite generates JSON descriptions of message fields, IDTO classes, enums and
> message IDs. Today, contributors must update and commit these files. A
> contributor may only learn about this requirement after CI runs near the end
> of review.
> We want CI to generate the descriptions and show the changes before merge,
> without an extra JSON commit. A JSON diff means that the wire format or
> registrations need review. It does not prove that compatibility is broken.
> Compatibility tests and manual review remain necessary.
> h2. Scope of this ticket
> # Remove generated JSON files from {{modules/wire-compatibility-check/wire/}}
> in Git, including {{ids.json}}.
> # Generate and collect the files under
> {{modules/wire-compatibility-check/target/classes/META-INF/ignite-wire/}}.
> # Remove the CI check that requires generated JSON changes to be committed.
> # Compare descriptions from two builds during the normal PR checks and
> publish their diff in GitHub Actions Job Summary.
> # Keep the existing checks for missing message descriptions and descriptions
> not collected by the module.
> # Add short local generation instructions in
> {{modules/wire-compatibility-check/DEVNOTES.md}}.
> The JSON schema, message ID assignment and serialization code do not change
> in this ticket.
> h2. When the comparison runs
> The comparison is a separate job, {{check-wire}}, named {{Compare wire format
> descriptions}}, in the existing {{.github/workflows/commit-check.yml}}. It
> runs together with the other PR checks. It does not depend on the result of
> {{check-java}}. Use the default GitHub Actions timeout; the earlier custom
> 30-minute limit was removed.
> Use the existing {{pull_request}} trigger and repository rules for approving
> external contributor runs. No separate manual action is required to run this
> comparison. The job is skipped for {{push}} events because they do not
> provide the PR context. The normal checks should be run early enough for the
> reviewer to read the diff before final approval.
> There is no target-branch name filter. PRs to {{master}} and release branches
> use the same rule. This extends the original ticket description, which
> mentioned only {{master}}.
> h2. Which revisions are compared
> Compare the target branch snapshot with the result of a test merge of the PR:
> ||Side||Revision||
> | Base | {{github.event.pull_request.base.sha}} |
> | PR merge | {{github.sha}} for the {{pull_request}} event |
> The default {{actions/checkout}} behavior for a PR checks out this merge
> revision. Build the two revisions in separate directories. Record both SHAs
> in the Summary.
> This shows what merging the PR would change relative to its target branch at
> the time of the event. For example, if the target branch already contains a
> wire change absent from the PR head, that change is present on both sides and
> is not reported as a change made by the PR.
> Do not compare the target branch directly with the PR head. Do not use
> merge-base versus PR head: that was an earlier option, and the agreed choice
> is now target versus PR merge. A report describes its recorded revisions; a
> later target-branch update does not make that report a check of the new
> target tip.
> h2. Generation and collection
> Use Java 17 and the Maven wrapper. For each revision, build
> {{wire-compatibility-check}} and its required modules:
> {code:bash}
> ./mvnw -B -ntp clean test-compile -pl modules/wire-compatibility-check -am -T
> 1C
> {code}
> Use {{test-compile}} because the reactor needs test classes. It does not
> execute the tests, so CI does not need {{-DskipTests}}. Keep {{clean}} to
> avoid files left by an earlier build. Delete the old
> {{modules/wire-compatibility-check/wire/}} directory before each build as
> well, so the comparison cannot use JSON left in Git by an older revision.
> New revisions collect JSON and write {{ids.json}} under
> {{target/classes/META-INF/ignite-wire/}} in the module. {{ids.json}} maps
> message IDs to message classes and must be included in the directory
> comparison.
> The base of this PR still generates files into the old {{wire/}} directory.
> Support both layouts: use the new directory when it contains {{ids.json}};
> otherwise use {{wire/}}. These must be freshly generated files. The separate
> shell check that failed explicitly on missing {{ids.json}} was removed by
> agreement. The remaining file test selects the directory; it is not a general
> validation of old generators.
> Keep the existing {{MessageIdsWriter}} validation for registered messages
> without descriptions. Keep {{Check all wire format descriptions are
> collected}} in {{check-java}}, updated to check the new collection directory.
> It still fails when a built module produces a description that was not
> collected. Do not replace these checks with warnings.
> h2. Results shown to the reviewer
> Compare the complete directories, including added and removed files and
> {{ids.json}}.
> ||Result||Expected behavior||
> | No differences | The job succeeds. Summary says {{No wire format changes.}}
> |
> | Differences found | The job succeeds with a warning. Summary shows the diff
> preview. |
> | Build, collection or comparison error | The relevant job fails. An error
> must not be reported as an empty diff. |
> Show at most the first 200,000 bytes of the diff in Summary. When there are
> differences, upload the complete {{wire.diff}} as the {{wire-diff}} artifact
> with 14-day retention. The preview must point the reviewer to the full
> artifact.
> The presence of a diff is informational and does not fail the job. Errors
> from the build or from {{diff}} itself must still fail it. Use read-only
> repository permissions and do not persist checkout credentials.
> h2. Local use and limits
> {{modules/wire-compatibility-check/DEVNOTES.md}} must explain why the JSON
> files exist, give a runnable command from the project root, state the output
> directory and tell contributors not to commit the generated files. The root
> {{DEVNOTES.txt}} stays unchanged.
> Old release branches may lack the module, a generator or message ID
> generation. Removing the target-branch filter does not add these features to
> old branches. Support for each such branch needs a separate check; this
> ticket does not promise full wire or ID comparison for all historical
> releases.
> The check compares generated descriptions. It is not a semantic compatibility
> analyzer and cannot guarantee detection of every change in hand-written
> serialization. A PR with merge conflicts may have no {{pull_request}}
> test-merge run; this ticket does not add another trigger for that case.
> h2. Acceptance scenarios
> * A contributor changes a message and pushes only source changes. CI
> generates the JSON; no JSON update commit is required.
> * A wire change produces a warning, a Summary preview with both SHAs and a
> full diff artifact. A change that leaves JSON unchanged produces the
> no-change message.
> * A message registration change appears in {{ids.json}} in the diff.
> * A PR whose branch is behind its target is compared as target versus PR
> merge, so changes already in the target are not reported as PR changes.
> * A PR to another target branch uses that branch's SHA. Validate this with a
> branch that has the required generators.
> * Both the old {{wire/}} layout and the new {{target/}} layout are generated
> from scratch and can be compared during this transition.
> * A build failure, an uncollected description or a technical comparison error
> produces a failed check, not a successful no-change report.
> * The local command and output path are documented. Generated JSON remains
> outside Git.
> h2. Follow-up: compatibility label through workflow_run
> Label changes are deliberately outside IGNITE-29261. This PR does not change
> {{check-protected-classes.yml}}, publish bot comments, add write permissions
> or change the PR description.
> The next task should replace the current Java-text rule for the
> {{compatibility}} label with the JSON comparison result. After a successful
> check of the current PR head, add the label when there is a diff and remove
> it when there is no diff. A failed or cancelled check, or a result for an
> older PR head, must not change the label.
> The intended event chain is:
> # The existing {{pull_request}} job builds, compares and publishes its result
> as data in an artifact. A successful no-change result must also be available
> to the label workflow; absence of a diff artifact alone is not proof of
> success.
> # A separate {{workflow_run}} workflow with {{types: [completed]}} reads that
> result, checks that it belongs to the current open PR head and updates the
> label. Its code comes from the default branch. It must not check out, build
> or execute PR code or artifact contents.
> ASF documents the replacement of {{pull_request_target}} with this split
> between a read-only build and a trusted workflow with write permissions. The
> new workflow must exist on the default branch before the event chain can be
> tested. The old label writer must be retired as part of that task so that two
> rules do not control the same label.
> Anton's earlier proposal used {{workflow_run: requested}} to set a label from
> source-file changes before the build. Our later agreement uses the JSON
> result, so it needs {{completed}}. Neither version requires moving the Maven
> comparison itself to {{workflow_run}}.
> Bot comments, automatic JSON commits, a semantic compatibility analyzer and
> generator backports to old release branches are not part of this ticket. A
> semantic analyzer remains a possible later step, not an agreed implementation
> requirement.
> h2. References
> * [Broader v2 proposal on the Ignite dev
> list|https://lists.apache.org/thread.html/hq3d8opxscww09pobvqfvwgc1d0v9cls].
> The decisions above record the later choices and the smaller scope of this
> ticket.
> * [ASF GitHub Actions
> policy|https://infra.apache.org/github-actions-policy.html].
> * [GitHub workflow
> events|https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows],
> including {{pull_request}} and {{workflow_run}}.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
