wombatu-kun commented on code in PR #20448: URL: https://github.com/apache/hudi/pull/20448#discussion_r4396909756
########## .claude/skills/build-hudi/SKILL.md: ########## @@ -0,0 +1,176 @@ +--- +name: build-hudi +description: >- + Use when compiling, packaging, running tests, or running checkstyle in apache/hudi (`master` is + 1.x / 1.3.0-SNAPSHOT, `branch-0.x` is 0.x / 0.16.0-SNAPSHOT), when deciding which JDK or Maven + profiles a branch needs, or when a build fails on `invalid source release: 11`, `DecimalWrapper + cannot find symbol`, `NoSuchMethodError: java.nio.ByteBuffer.flip()`, `AppClassLoader cannot be + cast to URLClassLoader`, a class file v55 error, or a dependency-resolution failure after a + branch switch. +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +# build-hudi — building & testing apache/hudi + +`master` is the 1.x line (`1.3.0-SNAPSHOT`); `branch-0.x` is the 0.x line (`0.16.0-SNAPSHOT`). +Both build the same way. The build is finicky about JDK and Maven profiles — follow these steps +exactly. Verify a fact with `git show <ref>:pom.xml` before trusting a stale memory. + +## The bases + +| Base | pom `java.version` (compile target) | Local JDK | CI JDK | Engine profiles | +|---|---|---|---|---| +| `master` | `11` | **11** | Temurin 11 for the main build (other jobs run 17 or 25) | `-Dspark3.3` … `-Dspark3.5`; `-Dspark4.0`+ needs JDK 17. `-Dflink1.18` … `-Dflink2.2` (default `flink2.2`) | +| `branch-0.x` | `1.8` | **11** | Temurin 8 (some jobs 17) | `-Dspark2.4` … `-Dspark3.5`; `-Dflink1.14` … `-Dflink1.18` | + +Defaults are Spark 3.5 / Scala 2.12. `-Dscala-2.13` applies to Spark bundles only, never Flink. On +`master` the metaserver is off by default: it needs `-Phudi-platform-service`, and +`-Pthrift-gen-source` to regenerate the Thrift sources. + +**Repo layout / worktrees.** Keep the clone on `master`. Do actual work in sibling `-wt-N` +worktrees (`hudi-wt-1`, …), never nested under the repo; see `develop-changes` for the worktree +convention. + +## Never run `mvn install` unless it is genuinely required + +Default to `mvn package` (or the `test` phase scoped with `-pl <module> -am`); do not run +`mvn install`. `-am` builds the sibling snapshots in-reactor and never writes to `~/.m2`. Only +use `install` when a task truly needs a locally published artifact (rare); confirm first. Two +reasons this matters: `install` pollutes the shared `~/.m2` and can corrupt concurrent reactor +builds running in other worktrees; and `~/.m2` reaches tens of GB on a working laptop, so do +not grow it needlessly. + +## 1. Pick the JDK + +Use **Java 11** locally on both bases. Set `JAVA_HOME` explicitly — on macOS +`export JAVA_HOME=$(/usr/libexec/java_home -v 11)` — and don't mix JDKs across compile and run. +Use a real Temurin/Corretto JDK (native arm64 on Apple silicon), not a Homebrew `openjdk@*`. + +`master` *requires* source level 11: its root pom sets `<java.version>11</java.version>`; +`hudi-io` overrides only `<target>8</target>` / `<release>8</release>` for old-Hive compatibility +and still inherits source 11. For `branch-0.x` the three numbers differ: the pom's compile target +is `1.8` (bytecode must stay Java-8 compatible), CI builds on Temurin 8, and the JDK you run +locally is 11. A construct that only exists in Java 9+ will compile locally there and then break +CI. + +Symptoms that identify a JDK mismatch: + +- `invalid source release: 11` — building `master` on JDK 8. It has no JDK-8 path. On an + incremental build over up-to-date classes this stays hidden ("Nothing to compile"), so only a + clean build shows it. +- `NoSuchMethodError: java.nio.ByteBuffer.flip()` — a local Java 8 run against a `~/.m2` + `spark-core_2.12-3.5.x` jar that was installed from a JDK 17 build. Clear and reinstall that + artifact. +- `AppClassLoader cannot be cast to URLClassLoader` — JDK 11 running the 0.x Hive 2.x test setup. + Run that module's tests on JDK 8, which is what `branch-0.x` CI uses. +- class file v55 errors — Flink 2.x jars (or other JDK 11+ artifacts) pulled into a JDK 8 build. +- JDK 17+/23 break independently (`ByteBuffer.limit` bytecode, Mockito inline mocking). + +## 2. Pass the engine profiles you mean + +A profile id the base does not define is silently ignored, and you get the default instead. Check +with `grep -oE "<id>flink[^<]*</id>" pom.xml | sort -u` if unsure. + +## 3. Full build (skip tests) + +```bash +mvn -B -ntp -q clean package -DskipTests -Dspark3.5 -Dscala-2.12 -T0.5C +``` + +Use `-T0.5C` (half a thread per core), **not** `-T 1C` or higher — on a laptop, fuller +parallelism overwhelms the processor, especially with other builds or worktrees active. Bundles +land in `packaging/*/target/`. + +## 4. `clean` after switching branches between 0.x and 1.x + +Switching between `master` and `branch-0.x` leaves stale `target/generated-sources` Avro classes +from the other line that no longer compile (`DecimalWrapper` / `TimeMicrosWrapper` +`cannot find symbol`, `SpecificRecordBuilderBase` constructor mismatch). Always `mvn clean` +after switching lines. A killed build can also leave corrupt incremental state that only +`mvn clean` on the affected modules fixes. + +## 5. Run tests + +Tests are JUnit 5 and split by tag: untagged = unit, `@Tag("functional")` / `functional-b` / +`functional-c` = functional, `IT*.java` = integration (failsafe). `-Punit-tests` or +`-Pfunctional-tests` selects one set; without either, surefire runs everything in scope. + +Sibling snapshots are **not** installed in `~/.m2`, so build the dependency modules in-reactor +with `-am` (a bare `mvn test -pl <module>` fails dependency resolution). Upstream `-am` modules +have no matching test, so add `-DfailIfNoTests=false`. + +```bash +export SPARK_LOCAL_IP=127.0.0.1 # required for Spark tests on macOS +mvn -B -ntp clean test -pl <module> -am -Dspark3.5 -Dscala-2.12 -T0.5C \ + -Dtest='TestClass#method' -DfailIfNoTests=false -Dmaven.test.redirectTestOutputToFile=true Review Comment: On master (surefire 3.5.4) `-DfailIfNoTests=false` is ignored once `-Dtest` is set, so this command stops at the first `-am` upstream module with `No tests matching pattern ... were executed!`. Could it use `-Dsurefire.failIfNoSpecifiedTests=false`, which section 5b and `run_perf_test.sh` already use? ########## .claude/skills/create-pr/SKILL.md: ########## @@ -0,0 +1,120 @@ +--- +name: create-pr +description: Use whenever changes to apache/hudi are being pushed for review, i.e. the user says "open/push/post/put up/raise a PR", "create a pull request", "send it for review", or similar. Use it for EVERY PR created or edited (any `gh pr create`/`gh pr edit`), including when opening the PR is only one step of a larger multi-part task; do not skip it because other work is in flight. +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +# Creating a PR + +Follow these steps: + +1. **Branch, in a worktree**: the branch must live in a dedicated git **worktree**, never the primary checkout (see `develop-changes` Step 0 — this should already have happened before the first edit). The worktree directory is a reused numbered pool slot named `<repo>-wt-<N>` (bare integer), NOT the branch or issue; the issue lives in the branch name, not the directory. If the change was mistakenly made in the primary checkout, move it into a worktree before committing. +2. **First commit**: use `git commit` with local git config for authoring and gpg signing. Do NOT add a Claude co-author trailer (CI rejects it). This first commit is the ONLY commit in the PR that carries the Conventional-Commit `type(scope):` prefix. +3. **Commits added after the PR is open** (review rounds): NO `fix:`/`feat:` prefix. Just write a plain sentence describing the change (e.g. `Gate the timestamp normalization to table version 8`). +4. **Push the PR** using the user's own GitHub account/handle — whichever `gh` is authenticated as (`gh api user --jq .login`); never hardcode a handle. **Always open as a DRAFT** (`gh pr create --draft`) unless the user explicitly says otherwise — the author self-reviews first, and some committers merge fast, so a non-draft PR risks being merged before that self-review. The user marks it ready themselves. +5. **PR description**: follow the repo's PR template **exactly**. Before writing the body, READ the template file from the repo you are targeting (`.github/PULL_REQUEST_TEMPLATE.md`, e.g. `git show <base>:.github/PULL_REQUEST_TEMPLATE.md`) and reproduce its section headers and checklist items **verbatim** — same wording, same case, same order. Fill each section under its real heading; do NOT invent, rename, merge, drop, or reorder sections, and do NOT invent your own checklist items (use the template's). + - For apache/hudi the current template is: `### Describe the issue this Pull Request addresses`, `### Summary and Changelog`, `### Impact`, `### Risk Level` (capital L), `### Documentation Update`, then a `### Contributor's checklist` with exactly these three items: `Read through [contributor's guide](...)`, `Enough context is provided in the sections above`, `Adequate tests were added if applicable`. Do NOT use the older `### Change Logs` heading or a lowercase `### Risk level` — the current template does not have them. + - When a subagent or a prior draft hands you a PR body, treat its prose as raw material only: re-fit it into the live template's exact sections before posting. A subagent draft that uses `### Change Logs`/`### Risk level`/custom checklist items is non-compliant; fix it, don't copy it. + - After creating or editing, re-read the posted body and confirm every template section header is present and correctly cased (a quick `grep` for each heading). Keep it succinct and reflective of the actual diff. Apply the voice in `develop-changes` **Writing for people** to all PR prose (title, body, review-comment replies): no em-dashes or `--` used as punctuation; restructure with commas, periods, colons, or parentheses. + - **Scale the body to the diff, and DEFAULT TO SHORT.** Small and medium PRs (a config/CI tweak, a one-liner, a bug fix, a contained refactor, anything a reviewer can hold in their head) get a high-level body of 2-5 sentences: why, what changed, and how it was verified. No `## Problem`/`## Changes`/`## Verification` headers, no bullet lists restating the diff, no tables, no background the reviewer can read off the code, and no narration of the investigation that produced the change. Length is not thoroughness: the reader wants orientation, not a report. Deep reasoning (root-cause traces, rejected alternatives, tool internals, measurements, counts) belongs in the issue or a review comment, not the description. Reserve a long, sectioned body for genuinely complex or subtle changes a reviewer cannot follow without it, and keep even those tight. **A large diff is not a licence to be comprehensive**: a 30-file CI bootstrap or a broad migration still gets a high-level summary, the few points a reviewer actually needs, and anything genuinely worth a second opinion. Do not enumerate every file, every deviation from the thing you ported, or every check you ran, and do not invent extra `###` sections to hold them: a reviewer who wants that detail reads the diff. When you catch yourself writing a per-item list of what changed and why, keep only the two or three items that would change a reviewer's mind and drop the rest. This applies whether or not the repo has a PR template; when there is a template, fill its sections at the same brevity and add no headings of your own. +6. **Iterating on an open PR**: add a NEW commit for each round of changes so reviewers can follow the history. Do NOT `git commit --amend` and do NOT force-push, unless the user explicitly instructs you to. +7. **Keep the PR description current**: after pushing any code change to an open PR, update the PR description (`gh pr edit`) so it always reflects the latest diff. Before editing, ALWAYS read the current description first (`gh pr view <n> --json body`) — the author may have edited it by hand. Preserve those and make a surgical change (fetch the body, modify the relevant part, write it back) rather than overwriting with a regenerated body. Write the new body to a file with the Write tool and pass `gh pr edit --body-file <path>`; do NOT pipe a body through a shell `sed`/`perl` here-string (a scripted in-place edit silently produced an empty file once and blanked the description — `gh pr edit` happily applies an empty body). After editing, re-read the body length to confirm it wasn't truncated. +8. **Reply to review comments after addressing them**: once the fix is pushed, reply to each comment thread concisely — start with `Done`/`Fixed`/`Addressed`, then at most one sentence of detail if it helps. No verbosity. Reply via `gh api repos/<owner>/<repo>/pulls/<n>/comments/<comment_id>/replies -f body='...'` (single-quote the body so backticks stay literal). + +## Self-review with a fresh agent before pushing + +Before the FIRST push of any PR (and before pushing any later round of changes), self-review the code first: + +1. **Spin up a NEW general-purpose subagent** (fresh context) and tell it to apply the `pr-correctness-review` skill to the exact diff, reviewing for correctness bugs and quality — a genuine second pair of eyes, not the same context that wrote it. Have it try to BREAK the change, not confirm it. +2. **Address its findings** in the working tree. +3. **Iterate the tests.** Any new change (from self-review, or later from reviewer comments) means re-running the added/affected tests and local functional/e2e validation again — change → test → review → repeat until the diff is clean and green. Do not push on the assumption a small follow-up edit is safe; re-validate it. + +Only push once self-review is addressed and the tests are green on the final diff. (Then per step 4, open as a draft.) + +## Squash before publishing + +By default, publish a PR as a **single commit**. If the branch accumulated multiple local +commits during development, squash them into one before the first push (e.g. +`git reset --soft <base-branch> && git commit`, preserving gpg signing and authoring). Review Comment: `git reset --soft <base-branch>` resets to the base's current tip, so once local `master` has moved past the branch point, the squash commit silently reverts every upstream commit in between. Could it reset to `$(git merge-base HEAD <base-branch>)` instead? ########## .claude/skills/prove-performance/scripts/pull_file_group_from_s3.sh: ########## @@ -0,0 +1,53 @@ +#!/usr/bin/env bash +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Download a Hudi file group (base file + its delta log files) from S3 to a local directory, for +# real-data performance analysis. All files of one file group in a partition directory share the +# same file id (e.g. record-index-0000-0); delta log files are hidden (leading dot). This matches +# both by file id so you get the base HFile/parquet AND the .log.* files. +# +# Usage: +# pull_file_group_from_s3.sh <s3-partition-uri> <file-id> [dest-dir] +# +# Example (metadata table record_index partition): +# pull_file_group_from_s3.sh \ +# s3://my-bucket/my-table/.hoodie/metadata/record_index/ \ +# record-index-0000-0 ./rli-fg +# +# Then analyze locally, e.g. with a gated real-data test of your own: +# run_perf_test.sh hudi-hadoop-common TestMyRealFileGroup \ +# -Dmy.real.fg.dir="$PWD/rli-fg/<file-id-dir>" +# +# Requires: awscli configured with read access to the bucket. +set -euo pipefail + +S3_PARTITION="${1:?s3 partition uri, e.g. s3://bucket/table/.hoodie/metadata/record_index/}" +FILE_ID="${2:?file id, e.g. record-index-0000-0}" +DEST="${3:-./$FILE_ID}" + +mkdir -p "$DEST" +echo "Pulling file group '$FILE_ID' from ${S3_PARTITION%/}/ -> $DEST" +# --recursive + include-by-file-id pulls the base and every delta log for this file group. +aws s3 cp "${S3_PARTITION%/}/" "$DEST/" --recursive \ + --exclude "*" --include "*${FILE_ID}*" Review Comment: `*${FILE_ID}*` also matches file groups whose id merely starts with this one: writers number file ids per task (`<prefix>-0`, `<prefix>-1`, ...), so `<prefix>-1` also pulls `<prefix>-10` and up once a task wrote 11 or more files. Would `*${FILE_ID}_*` work, since base and log file names both put `_` right after the file id? ########## .claude/skills/review-pr/references/reviewer-priorities.md: ########## @@ -0,0 +1,171 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +# What Hudi reviewers actually scrutinize + +Distilled from 30,530 review comments across 3,945 Apache Hudi PRs by five senior +reviewers: vinothchandar (project creator), nsivabalan, yihua, bvaradar, and danny0405 +(Flink integration lead, 6,729 comments). Where a rule is attributed, that reviewer +enforces it repeatedly. + +Loaded by `review-pr` Step 3. Companion: +[`invariant-checklist.md`](invariant-checklist.md) for the blocking gates. + +## Project shape + +- **Key modules**: `hudi-client` (write path), `hudi-common` (shared data structures, + timeline, metadata), `hudi-spark-datasource` (Spark integration), `hudi-flink` (Flink + integration, dedicated maintainer), `hudi-utilities` (HoodieStreamer ingestion). +- **Languages**: Java core, Scala for the Spark datasource / SQL integration. +- **Storage formats**: Parquet base files, Avro log files, custom log blocks. +- **Engine parity**: Spark and Flink are both first-class. Flink has its own write path, + compaction operators, CDC/changelog support, and streaming semantics. Config + declarations belong in `hudi-common`, not duplicated across the Spark and Flink modules. + +## Review priorities, in order + +1. **Thread safety and concurrency** — the most heavily scrutinized area. Reviewers + construct thread-interleaving diagrams to prove race conditions. Scrutinize shared + mutable state, lock ordering, heartbeat interactions, and timeline reload sequences. + Flink adds checkpoint threads and compaction operators racing with commits. +2. **Metadata table (MDT) correctness** — valid instant filtering, rollback propagation + from data table to MDT, compaction blocking rules, delta-commit consistency. Changes to + `HoodieBackedTableMetadataWriter` / `HoodieBackedTableMetadata` deserve deep inspection. + danny0405 flags where MDT compaction restrictions can be relaxed, since the MDT reader + already filters invalid instants. +3. **Schema evolution and backwards compatibility** — Avro schema changes must preserve + reader compatibility. Field additions need defaults; payload deserialization must handle + older formats; check `InternalSchema` handling and Spark/Flink-to-Avro type mappings. In + Flink, primary-key nullability differs from Spark. +4. **Error handling and failure modes** — "what happens if this fails mid-way?" Partial + failures must leave the table recoverable via rollback. For Flink streaming jobs, + checkpoint failure recovery paths are critical. +5. **Test coverage** — every new code path needs tests, covering both COW and MOR, edge + cases (empty inputs, schema mismatches, concurrent ops), and parameterized tests for + combinatorial scenarios. nsivabalan enumerates exact test matrices. +6. **Performance at scale** — Hudi runs on PB-scale tables with 10M+ files. Flag full-table + scans, N+1 patterns, unbounded in-memory collections, redundant timeline reloads, + missing `.persist()` on reused RDDs, and filesystem listing pressure (danny0405: read + the schema directly from files, not via `TableSchemaResolver`). +7. **Config management** — new properties use the `ConfigProperty` builder with `.key()`, + `.defaultValue()`, `.withDocumentation()`, `.sinceVersion()`; `.withAlternatives()` for + renames. Consolidate duplicate declarations across Spark/Flink into `hudi-common`. Flink + has split configs (separate scheduling vs execution for inline compaction) that must + stay consistent with the Spark equivalents. +8. **API stability and design** — public API changes require deprecating the old method + first. Separate transformation logic from container classes. Contracts must be + consistent (e.g. RDD persist/unpersist ownership in the write client). bvaradar + emphasizes clean separation of responsibilities in class hierarchies. + +## Must-check patterns + +Always flag these when they appear in a diff: + +- **Shared mutable state without synchronization** — look for `volatile`, `synchronized`, + `AtomicReference`, or an explicit lock on shared fields. +- **Missing null checks** on user configs, schema providers, filesystem calls, timeline + instant details. (Only where a realistic null path exists.) +- **Timeline operations without reload** — stale timelines produce incorrect rollback and + compaction decisions. +- **Avro `Schema` captured in a closure** — `org.apache.avro.Schema` is not serializable. + Flag any lambda / RDD / Flink operator capturing it; pass strings and reconstruct. +- **Unclosed resources** — `HoodieFileReader`, `HoodieLogFileReader`, input streams. +- **Missing test for a new code path** — new branches, methods, or configs. +- **Config property without `sinceVersion` or documentation.** +- **Public API change without deprecation** — `BaseHoodieWriteClient`, `HoodieWriteConfig`, + datasource options. +- **Hardcoded magic numbers** — thresholds, timeouts, batch sizes should be configurable. +- **MOR log/base file interactions** — verify ordering-field semantics and delete handling. +- **Flink rollback instant ordering** — rollback instants can carry timestamps greater than + pending commits, causing timeline-service sync issues. +- **Flink primary-key nullability** — Flink and Spark differ; flag code assuming uniform + nullability across engines. + +## Anti-patterns + +1. **Double-pass processing** — iterating the same collection twice where one pass suffices + (parsing then re-parsing a file name). +2. **Redundant timeline reloads** — repeated `metaClient.reloadActiveTimeline()` in one + operation path. +3. **Unsafe concurrent collection usage** — `HashMap` where `ConcurrentHashMap` is needed; + iterating a collection another thread mutates. +4. **Catching generic `Exception`/`Throwable`** — hides real failures. +5. **Reassigning method return values** — one variable holding both the original and the + transformed result. +6. **Duplicate code across COW/MOR or Spark/Flink** — logic that belongs in a shared base + class, copy-pasted into engine- or table-type-specific implementations. +7. **Missing `.withAlternatives()` on a renamed config.** +8. **Ignoring table-version differences** — code assuming one version, when layouts, + timeline formats, and MDT schemas differ across `HoodieTableVersion`. +9. **Filesystem listing where a metadata read suffices** — `TableSchemaResolver` or full + listing when the schema can be read from the file or MDT. + +## Code style + +- **Imports**: grouped by package (java, javax, scala, org.apache.hudi, third-party). No Review Comment: Three rules here contradict the build or the other skills: `style/checkstyle.xml` orders imports `org.apache.hudi, *, javax, java, scala` (an error on master), master logs through Lombok `@Slf4j` `log` (as the `port-fixes` drift table says), and `develop-changes` bans tracker ids in source while this asks for a JIRA on every TODO. Could these be aligned, since `review-pr` lets the reference win when the two disagree? ########## .claude/skills/review-pr/references/invariant-checklist.md: ########## @@ -0,0 +1,124 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +# Hudi invariant checklist + +Check every changed code path against these. A **BLOCKING** violation must be fixed before +merge; an **IMPORTANT** violation is a finding unless the surrounding code accepts the +tradeoff deliberately and says so. + +Loaded by `review-pr` Step 3. Companion: +[`reviewer-priorities.md`](reviewer-priorities.md) for what reviewers weight and why. + +## Breaking changes — BLOCKING + +- `@PublicAPIClass` constructor or name changes, `@PublicAPIMethod` signature changes → + breaking; needs an RFC / migration plan. +- Config key renames or removals without a `withAlternatives(...)` deprecation path. +- Config semantics silently changed — one config's change altering whether another takes + effect. +- Storage-format changes without versioning or backward compatibility: + - Avro schemas for timeline metadata (LSM, archived, active) + - Metadata table Avro schema + - Parquet footer metadata (bloom filters, key ranges) — must handle old **and** new encoding + - Log format header/footer additions — old versions must handle new keys gracefully + - Log block content serialization — the log block format must be versioned + - Base / log file naming conventions + - `.hoodie` metadata directory structure or paths + - Metadata table partition names — needs migration steps + - Record-to-file-group sharding within MDT partitions + - Record key / metafield encoding + +## Concurrency and data correctness — BLOCKING + +- Timeline operations (request + complete) happen **within** a lock, and the lock is + released on every path. +- The timeline is refreshed **inside** the lock's critical section before any check, so + concurrent completions since lock acquisition are visible. +- The timeline is refreshed before requesting new actions (outside the lock). +- No check-then-act on distributed storage (HDFS, S3, GCS) — those operations are not + atomic. +- Shared mutable state has correct `synchronized` / `volatile` / atomic access. +- Spark partition pruning, predicate pushdown, data skipping, index pruning, and schema + evolution paths must not silently drop or corrupt data. + +## Resource leaks — BLOCKING + +- `SparkSession`, `JavaSparkContext`, `HoodieTableMetaClient`, `FileSystem`, Review Comment: This BLOCKING gate asks for try-with-resources on objects that have no `close()` (`HoodieTableMetaClient`, `HoodieTable`) or that the changed code usually borrows rather than owns (a cached `FileSystem`, which `HoodieHadoopStorage.close` deliberately leaves open, and the caller's `SparkSession`/`JavaSparkContext`), so a review or self-review that follows it raises false blockers or closes shared state. Could the list keep only what the changed code opens itself (streams, readers, writers, record iterators)? ########## .claude/skills/port-fixes/SKILL.md: ########## @@ -0,0 +1,128 @@ +--- +name: port-fixes +description: Use when one Hudi fix has to land on both apache/hudi `master` (1.x) and `branch-0.x`, or when a PR fails the `validate-pr-and-source` co-author or PR-template check; when a metadata-table test passes locally but reproduces nothing; or when a cherry-pick compiles cleanly and then tests nothing on the other branch. +--- + +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +--> + +# port-fixes — one defect, two bases, two red runs + +## Overview + +One defect, two PRs, two different environments. The bases diverge in Java target, timeline +API, and metadata-table instant scheme, so a straight cherry-pick compiles and then silently tests +nothing. Verify red on **each** base separately. + +## The two bases + +Both are branches of the one `hudi` clone. Do the actual work in **sibling** git worktrees +(`hudi-wt-1`, `hudi-wt-2`, …), siblings of the clone, never nested inside it. + +| base | line | notes | +|---|---|---| +| `apache/hudi` **master** | 1.x | `-Dscala-2.12 -Dspark3.5` (default Flink profile) | +| `apache/hudi` **branch-0.x** | 0.x | `-Dscala-2.12 -Dspark3.5`; Flink profiles stop at `flink1.18` | + +**`build-hudi` is authoritative for each base's `<java.version>`, local JDK, and CI JDK** — look +them up there rather than here, so the two cannot drift. + +*Troubleshooting:* a local Java 8 run can die with `NoSuchMethodError: java.nio.ByteBuffer.flip()` +when `~/.m2` holds a `spark-core_2.12-3.5.x` jar that was installed from a JDK 17 build. The fix is +to clear and reinstall that artifact. Running the same build on Temurin 11 also makes it go away +(Java 8-target bytecode runs there), but that is a workaround, not the default. + +## Order of operations + +1. Fix on **master** first, with its regression test. +2. Backport to **branch-0.x** only when the defect exists there. Open it as its own PR and name the + master PR in its body. +3. Each PR gets its own red/green run on its own base (below). + +## Traps that cost real time + +**Metadata-table init instants differ by table version.** 0.x always appends a three-digit suffix to +the data instant. 1.x `HoodieBackedTableMetadataWriter#generateUniqueInstantTime` derives them from +`SOLO_COMMIT_TIMESTAMP` (sorts *below* every data instant); only +`HoodieBackedTableMetadataWriterTableVersionSix` still appends the suffix. A test needing the suffixed +scheme on a 1.x base must set `hoodie.write.table.version=6` **and** `withWriteTableVersion(6)`, +**and** construct the writer through `SparkMetadataWriterFactory.create(..., tableConfig)` — calling +`SparkHoodieBackedTableMetadataWriter.create(...)` directly always yields the v8 writer, so the test +passes while reproducing nothing. Assert `HoodieTableVersion.SIX` as a precondition. + +**A bypass flag passed as `-DargLine` never reaches the fork.** Hudi's pom sets surefire's argLine to +`@{argLine}` for jacoco, so `-DargLine=-Dfoo=true` is discarded and the "red" run passes. Use a plain +`-Dfoo=true`. Prove the flag is live by first making the bypass *throw*, then switch it to the +pass-through behaviour and re-run. + +**apache/hudi rejects Claude co-author trailers.** The `validate-pr-and-source` job +(`.github/workflows/validate_source.yml`) fails any commit matching `co-authored-by: claude ...` or +`<[email protected]>`. Rewrite the offending commit and force-push; this is the one case where +`create-pr`'s no-force-push rule gives way. + +**apache/hudi enforces PR template section order.** The same job requires +`### Describe the issue this Pull Request addresses` first, then Summary and Changelog / Impact / +Risk Level / Documentation Update / Contributor's checklist. Titles are conventional-commit +(`fix(scope): ...`); no JIRA. + +## API drift, 0.x → 1.x + +| 0.x | 1.x | +|---|---| +| `instant.getTimestamp()` | `instant.requestedTime()` | +| `metaClient.getBasePathV2()` | `metaClient.getBasePath()` | +| `HoodieTimeline.compareTimestamps/LESSER_THAN_OR_EQUALS` | `InstantComparison.*` | +| `HoodieActiveTimeline.parseDateFromInstantTime/formatDate` | `TimelineUtils.*` | +| `HoodieMetadataConfig.Builder.withEnableRecordIndex` | `withEnableGlobalRecordLevelIndex` | +| `create(Configuration, ...)` | `create(StorageConfiguration<?>, ...)` | +| `LOG` field | Lombok `@Slf4j` `log` on master | + +Do not swap `parseDateFromInstantTime` for `parseDateFromInstantTimeSafely` — the latter maps +`NOT_PARSABLE_TIMESTAMPS` to `new Date(Integer.parseInt(...))`, an epoch-era value. + +Checkstyle import ordering is stricter on master than on 0.x; run `mvn checkstyle:check -pl <module>` +per base before pushing. + +## Per-base verification loop + +``` +mvn -q clean package -pl hudi-utilities -am -DskipTests <profiles> +mvn test -pl hudi-utilities -am -Punit-tests -Dtest="<Class>#<newTests>" -DfailIfNoTests=false <profiles> # expect GREEN Review Comment: With `-Punit-tests` this runs every ScalaTest suite in `hudi-spark`, which is in the `hudi-utilities -am` chain (scalatest ignores `-Dtest`, and `-DskipUTs=true` would skip surefire too under that profile), and it repeats the `-DfailIfNoTests=false` flag that surefire 3.x ignores. Could it point at `build-hudi` section 5 instead of restating the command? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
