diff --git a/.claude/agents/code-reviewer.md b/.claude/agents/code-reviewer.md index a2db89d2a893..0b2557f89e6f 100644 --- a/.claude/agents/code-reviewer.md +++ b/.claude/agents/code-reviewer.md @@ -1,6 +1,6 @@ --- name: code-reviewer -description: 'Independent multi-agent code reviewer for Apache Pinot. Spawns 8 domain-specialized sub-reviewers in parallel (config-backcompat, concurrency-state, architecture, performance, correctness-nulls, testing, naming-api, process-scope), aggregates their findings, de-duplicates by (principle, file, line), and produces a consolidated report ranked by severity. Use proactively after writing or modifying code, especially before commits and PRs.\n\n**IMPORTANT — Minimal context rule:** Caller provides ONLY (1) review scope (e.g. "unstaged changes", "branch vs master", specific paths) and (2) a one-line change description. Never pass opinions or analysis — each sub-reviewer forms its judgment independently by reading the code and the principles KB.\n\nExamples:\n\nuser: "I''ve added the new authentication feature. Can you check if everything looks good?"\nassistant: Invokes code-reviewer with prompt: "Review unstaged changes. Change: added authentication feature to broker."\n\nMinimal context — just scope and one-line summary. No opinions about the code.\n\n\n\nassistant just wrote a utility function and wants to validate it.\nassistant: Invokes code-reviewer with prompt: "Review unstaged changes in pinot-common. Change: added partition ID utility function."\n\nProactive review after writing code. No analysis of what might be wrong.\n\n\n\nuser: "I think I''m ready to create a PR for this feature"\nassistant: Invokes code-reviewer with prompt: "Review all changes on this branch vs master. Change: new config validation for upsert tables."\n\nPre-PR review. Scope is branch diff, description is one line.\n\n' +description: Review Apache Pinot diffs or designs using applicable correctness, compatibility, and safety checks. Delegate substantial independent checks when useful. model: inherit color: red --- diff --git a/.claude/skills/bench-compare/SKILL.md b/.claude/skills/bench-compare/SKILL.md index 1b79a6f928bb..1ef2f4487aea 100644 --- a/.claude/skills/bench-compare/SKILL.md +++ b/.claude/skills/bench-compare/SKILL.md @@ -16,7 +16,7 @@ # specific language governing permissions and limitations # under the License. name: bench-compare -description: Run a Pinot JMH benchmark twice — once on a baseline commit, once with the current changes — and report both sets of results side by side. Uses a git worktree so the user's working tree is never disturbed. +description: Compare baseline and current Pinot JMH benchmark results using an isolated baseline worktree. --- # /bench-compare diff --git a/.claude/skills/flaky-analyze/SKILL.md b/.claude/skills/flaky-analyze/SKILL.md index 83476fdb61fb..17d05b3642d3 100644 --- a/.claude/skills/flaky-analyze/SKILL.md +++ b/.claude/skills/flaky-analyze/SKILL.md @@ -16,7 +16,7 @@ # specific language governing permissions and limitations # under the License. name: flaky-analyze -description: Pull recent GitHub Actions failures for a Pinot test class and analyze whether they share a root cause. Uses the gh CLI. Surfaces stack traces, failure patterns, and a candidate hypothesis — does not auto-fix. +description: Investigate intermittent Pinot test failures in GitHub Actions and report evidence and likely causes. --- # /flaky-analyze diff --git a/.claude/skills/precommit/SKILL.md b/.claude/skills/precommit/SKILL.md index 2a155e2dcc9b..490514a96db8 100644 --- a/.claude/skills/precommit/SKILL.md +++ b/.claude/skills/precommit/SKILL.md @@ -16,7 +16,7 @@ # specific language governing permissions and limitations # under the License. name: precommit -description: Run Pinot's mandatory pre-commit checks (spotless, license, checkstyle) and compiler warning checks on only the modules affected by the current diff. Auto-fixes what it can, reports what it can't. +description: Validate and fix Pinot pre-commit checks on affected modules before a commit or PR. --- # /precommit diff --git a/.claude/skills/quickstart/SKILL.md b/.claude/skills/quickstart/SKILL.md index 7d0688b306cf..fc6fbd5a524b 100644 --- a/.claude/skills/quickstart/SKILL.md +++ b/.claude/skills/quickstart/SKILL.md @@ -16,7 +16,7 @@ # specific language governing permissions and limitations # under the License. name: quickstart -description: Launch a local Pinot quickstart cluster (batch, hybrid, streaming, upsert, etc.) with the right script, building the binary distribution first if needed. +description: Start and verify a local Pinot quickstart using the modes supported by the current checkout. --- # /quickstart diff --git a/.claude/skills/review-architecture/SKILL.md b/.claude/skills/review-architecture/SKILL.md index 85976bb1c235..dacd63dacda1 100644 --- a/.claude/skills/review-architecture/SKILL.md +++ b/.claude/skills/review-architecture/SKILL.md @@ -1,6 +1,6 @@ --- name: review-architecture -description: Review Apache Pinot diffs for architectural concerns — module boundaries, SPI vs. impl separation, circular deps, misplaced logic (broker code in server, server code in controller), abstraction choice (interface vs. abstract class), plugin layering, and layering violations between pinot-spi / pinot-common / pinot-core / pinot-segment-spi / pinot-segment-local. Trigger keywords — new interface, abstract class, package move, module, SPI, broker-server boundary, plugin, shaded. +description: Review Apache Pinot architecture when module dependencies, SPI boundaries, abstractions, or class placement change. domain: kb/code-review-principles.md#3-code-architecture--module-design triggers: - diff adds/moves classes across module boundaries diff --git a/.claude/skills/review-concurrency-state/SKILL.md b/.claude/skills/review-concurrency-state/SKILL.md index 59616935486d..338e64f82c53 100644 --- a/.claude/skills/review-concurrency-state/SKILL.md +++ b/.claude/skills/review-concurrency-state/SKILL.md @@ -1,6 +1,6 @@ --- name: review-concurrency-state -description: Review Apache Pinot diffs for concurrency, state management, visibility, atomic state transitions, lock changes, Helix IdealState updates, upsert metadata safety, consumer/stream ingestion races, and shared-observer correctness. Trigger keywords — synchronized, volatile, AtomicReference, ConcurrentHashMap, ReentrantLock, StampedLock, Helix, IdealState, ZkClient, version-checked write, upsert metadata, consumer coordinator, stream partition. +description: Review Apache Pinot concurrency and state safety when locks, shared callbacks, lifecycle, or distributed metadata change. domain: kb/code-review-principles.md#2-state-management--concurrency triggers: - diff adds/removes synchronized / volatile / Atomic* / lock types diff --git a/.claude/skills/review-config-backcompat/SKILL.md b/.claude/skills/review-config-backcompat/SKILL.md index 651f92498786..d88887890c1b 100644 --- a/.claude/skills/review-config-backcompat/SKILL.md +++ b/.claude/skills/review-config-backcompat/SKILL.md @@ -1,6 +1,6 @@ --- name: review-config-backcompat -description: Review Apache Pinot diffs for configuration and backward-compatibility risks. Covers config key renames, SPI signature changes, schema/enum additions, feature-flag defaults, multi-level config override validation, rolling-upgrade safety, REST/JSON/Protobuf field evolution, and segment/DataTable format versioning. Trigger keywords — config key, config rename, SPI, feature flag, enum, schema type, DataTable version, segment version, Protobuf, Thrift, REST endpoint, @Deprecated, mixed-version, rolling upgrade, ZooKeeper schema. +description: Review Apache Pinot compatibility when configs, public APIs, SPI contracts, or serialized formats change. domain: kb/code-review-principles.md#1-configuration--backward-compatibility triggers: - diff touches config constants (*ConfigConstants.java, *Config.java) diff --git a/.claude/skills/review-correctness-nulls/SKILL.md b/.claude/skills/review-correctness-nulls/SKILL.md index b4a330de8728..ba9c4029f91c 100644 --- a/.claude/skills/review-correctness-nulls/SKILL.md +++ b/.claude/skills/review-correctness-nulls/SKILL.md @@ -1,6 +1,6 @@ --- name: review-correctness-nulls -description: Review Apache Pinot diffs for correctness issues — null handling, type dispatch, numeric precision (INT/LONG/BIG_DECIMAL), exhaustive switch coverage for DataType / IndexType, resource leaks in close/destroy paths, off-by-one errors in row iteration, and silent wrong-result risks. Trigger keywords — null, Nullable, Optional, getStoredType, DataType switch, IndexType switch, close, destroy, realtime persist, precision, BigDecimal, isNullable, null vector. +description: Review Apache Pinot correctness when null handling, numeric types, dispatch, or resource lifecycles change. domain: kb/code-review-principles.md#5-correctness--safety triggers: - diff touches null-vector / null-bitmap / null-enabled code paths diff --git a/.claude/skills/review-naming-api/SKILL.md b/.claude/skills/review-naming-api/SKILL.md index b414ca38ffa3..7b5e9a38e2a0 100644 --- a/.claude/skills/review-naming-api/SKILL.md +++ b/.claude/skills/review-naming-api/SKILL.md @@ -1,6 +1,6 @@ --- name: review-naming-api -description: Review Apache Pinot diffs for naming, API design, and public-surface hygiene — method and class names; REST JSON field names; SPI method names; enum constant names (permanent); consistency with existing patterns; fully-qualified class names inline (disallowed); Javadoc on new public classes. Trigger keywords — public API, SPI, REST, @JsonProperty, enum name, class rename, method rename, Javadoc. +description: Review Apache Pinot naming and API conventions when public names, signatures, or documentation change. domain: kb/code-review-principles.md#7-naming--api-design triggers: - diff adds or renames public classes/methods/fields diff --git a/.claude/skills/review-performance/SKILL.md b/.claude/skills/review-performance/SKILL.md index 717a8d09960b..3427026dc7a0 100644 --- a/.claude/skills/review-performance/SKILL.md +++ b/.claude/skills/review-performance/SKILL.md @@ -1,6 +1,6 @@ --- name: review-performance -description: Review Apache Pinot diffs for performance regressions in hot paths — per-row allocations, autoboxing, virtual dispatch in tight loops, large synchronized sections on the query path, unnecessary ByteBuffer copies, string concat in loops, and missing fast-paths for common types. Trigger keywords — TransformOperator, FilterOperator, ForwardIndexReader, segment scan, per-row, query hot path, allocation, autoboxing, JMH, benchmark. +description: Review Apache Pinot performance when hot paths change or a PR makes benchmark or efficiency claims. domain: kb/code-review-principles.md#4-performance--efficiency triggers: - diff touches pinot-query-runtime/**/operator/**, pinot-core/**/operator/**, transform/aggregation function diff --git a/.claude/skills/review-process-scope/SKILL.md b/.claude/skills/review-process-scope/SKILL.md index e46567f28196..a732dc91af42 100644 --- a/.claude/skills/review-process-scope/SKILL.md +++ b/.claude/skills/review-process-scope/SKILL.md @@ -1,6 +1,6 @@ --- name: review-process-scope -description: Review Apache Pinot diffs for process and scope discipline — PR size, single-concern commits, commit message clarity, referenced issues/PRs on reverts, anti-patterns like "add retry to fix flake", labels (backward-incompat), rolling-upgrade notes, and TODO hygiene. Trigger keywords — revert, retry, flake, TODO, backward-incompat, rolling upgrade, PR description. +description: Review Apache Pinot change scope and process when assessing PR descriptions, reverts, retries, or compatibility notes. domain: kb/code-review-principles.md#8-process--scope triggers: - diff is > ~500 changed lines or spans > 4 modules diff --git a/.claude/skills/review-testing/SKILL.md b/.claude/skills/review-testing/SKILL.md index 6f996555b4dc..a5d22ab09e10 100644 --- a/.claude/skills/review-testing/SKILL.md +++ b/.claude/skills/review-testing/SKILL.md @@ -1,6 +1,6 @@ --- name: review-testing -description: Review Apache Pinot diffs for test coverage and test quality — positive + negative cases, real dictionaries vs mocks, rolling-upgrade / mixed-version tests, null-handling toggle coverage, exhaustive type coverage for aggregators/operators, integration-test base-class choice (reject standalone clusters unless special setup is needed; prefer `CustomDataQueryClusterIntegrationTest`), assertion quality, and regression tests that reproduce the bug. Trigger keywords — Test, TestNG, JUnit, Mockito, mock, integration test, assertEquals, assertThrows, regression, null handling test, mixed version, CustomDataQueryClusterIntegrationTest, BaseClusterIntegrationTest. +description: Review Apache Pinot regression coverage and test quality when behavior or tests change. domain: kb/code-review-principles.md#6-testing-strategies triggers: - diff adds or modifies any src/test/** file diff --git a/.claude/skills/run-test/SKILL.md b/.claude/skills/run-test/SKILL.md index 07d7da995fb7..cb75b95510f0 100644 --- a/.claude/skills/run-test/SKILL.md +++ b/.claude/skills/run-test/SKILL.md @@ -16,7 +16,7 @@ # specific language governing permissions and limitations # under the License. name: run-test -description: Run a single Pinot JUnit/TestNG test class by name. Auto-detects the owning Maven module and builds the correct ./mvnw invocation, including the integration-test flags when needed. +description: Run a named Pinot test class or method in its owning Maven module. --- # /run-test diff --git a/AGENTS.md b/AGENTS.md index 0da271e85a11..efdaa51c8757 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,72 +43,26 @@ repo. It is intentionally short and focused on day-to-day work. - pinot-integration-tests: end-to-end validation suites. - pinot-distribution: packaging artifacts. -## pinot-plugins modules -- pinot-input-format: input format plugin family. - - pinot-arrow: Apache Arrow input format support. - - pinot-avro: Avro input format support. - - pinot-avro-base: shared Avro utilities and base classes. - - pinot-bson: MongoDB BSON input format support. - - pinot-clp-log: CLP log input format support. - - pinot-confluent-avro: Confluent Schema Registry Avro input support. - - pinot-confluent-json: Confluent Schema Registry JSON input support. - - pinot-confluent-protobuf: Confluent Schema Registry Protobuf input support. - - pinot-orc: ORC input format support. - - pinot-json: JSON input format support. - - pinot-parquet: Parquet input format support. - - pinot-csv: CSV input format support. - - pinot-thrift: Thrift input format support. - - pinot-protobuf: Protobuf input format support. -- pinot-file-system: filesystem plugin family. - - pinot-adls: Azure Data Lake Storage (ADLS) filesystem support. - - pinot-hdfs: Hadoop HDFS filesystem support. - - pinot-gcs: Google Cloud Storage filesystem support. - - pinot-s3: Amazon S3 filesystem support. -- pinot-batch-ingestion: batch ingestion plugin family. - - pinot-batch-ingestion-common: shared batch ingestion APIs and utilities. - - pinot-batch-ingestion-spark-base: shared Spark ingestion base classes. - - pinot-batch-ingestion-spark-3: Spark 3 ingestion implementation. - - pinot-batch-ingestion-hadoop: Hadoop MapReduce ingestion implementation. - - pinot-batch-ingestion-standalone: standalone batch ingestion implementation. -- pinot-stream-ingestion: stream ingestion plugin family. - - pinot-kafka-base: shared Kafka ingestion base classes. - - pinot-kafka-3.0: Kafka 3.x ingestion implementation. - - pinot-kafka-4.0: Kafka 4.x ingestion implementation. - - pinot-kinesis: AWS Kinesis ingestion implementation. - - pinot-pulsar: Apache Pulsar ingestion implementation. -- pinot-minion-tasks: minion task plugin family. - - pinot-minion-builtin-tasks: built-in minion task implementations. -- pinot-metrics: metrics reporter plugin family. - - pinot-dropwizard: Dropwizard Metrics reporter implementation. - - pinot-yammer: Yammer Metrics reporter implementation. - - pinot-compound-metrics: compound metrics implementation. -- pinot-segment-writer: segment writer plugin family. - - pinot-segment-writer-file-based: file-based segment writer implementation. -- pinot-segment-uploader: segment uploader plugin family. - - pinot-segment-uploader-default: default segment uploader implementation. -- pinot-environment: environment provider plugin family. - - pinot-azure: Azure environment provider implementation. -- pinot-timeseries-lang: time series language plugin family. - - pinot-timeseries-m3ql: M3QL language plugin implementation. -- assembly-descriptor: Maven assembly descriptor for plugin packaging. +When locating a plugin implementation, consult [kb/plugin-modules.md](kb/plugin-modules.md). ## Build and test -- Build JDK: Use JDK 21+ for Pinot services and the default build; client and SPI artifacts still target Java 11 bytecode. -- Runtime JRE: Broker/server/controller/minion run on Java 21+. -- Default build: `./mvnw clean install` -- Faster dev build: `./mvnw verify -Ppinot-fastdev` -- Full binary/shaded build: - `./mvnw clean install -DskipTests -Pbin-dist -Pbuild-shaded-jar` -- Build a module with deps: `./mvnw -pl pinot-server -am test` -- Single test example: `./mvnw -pl pinot-segment-local -Dtest=RangeIndexTest test` -- Quickstart (after build): `build/bin/quick-start-batch.sh` +- Build JDK: Use JDK 25+ for Pinot services and the default build; client and SPI artifacts still target Java 11 bytecode. +- Runtime JRE: Broker/server/controller/minion run on Java 25+. +- Default to affected modules and their dependencies: `./mvnw -pl -am test`. +- Faster module build: `./mvnw -pl -am verify -Ppinot-fastdev`. This profile skips style and license checks; + it does not replace applicable pre-push validation. +- Single test example: `./mvnw -pl pinot-segment-local -am -Dtest=RangeIndexTest -Dsurefire.failIfNoSpecifiedTests=false test`. + See [kb/skills/run-test.md](kb/skills/run-test.md) for dependency availability and evidence reuse. +- Full reactor build, when the task requires it: `./mvnw clean install`. +- Full binary/shaded distribution, when required: `./mvnw clean install -DskipTests -Pbin-dist -Pbuild-shaded-jar`. +- Local cluster startup: follow [kb/skills/quickstart.md](kb/skills/quickstart.md) for the smallest required build and readiness checks. ## Integration tests - Single integration test example: `./mvnw -pl pinot-integration-tests -am -Dtest=OfflineClusterIntegrationTest -Dsurefire.failIfNoSpecifiedTests=false test` ## Coding conventions and hygiene - Add class-level Javadoc for new classes; describe behavior and thread-safety. -- Use Javadoc comments with either `/** ... */` or `///` syntax (per JEP-467); service code targets Java 21 by default. +- Use Javadoc comments with either `/** ... */` or `///` syntax (per JEP-467); service code targets Java 25 by default. - Keep license headers on all new source files. - Use `./mvnw license:format` to add headers to new files. - Preserve backward compatibility across mixed-version broker/server/controller. @@ -137,13 +91,9 @@ repo. It is intentionally short and focused on day-to-day work. - Run `./mvnw license:check` to validate license headers. ## Pre-commit checks -Before pushing a commit, always run the following checks on the affected modules and fix any failures: -1. `./mvnw spotless:apply -pl ` — auto-format code. -2. `./mvnw checkstyle:check -pl ` — validate style conformance. -3. `./mvnw license:format -pl ` — add missing license headers to new files. -4. `./mvnw license:check -pl ` — verify all files have correct license headers. - -Do not push until all four checks pass cleanly. +Before pushing, follow [kb/skills/precommit.md](kb/skills/precommit.md) for affected-module formatting, license headers, +checkstyle, and license validation. That procedure defines applicability, execution order, evidence reuse, and when +compiler warning checks are needed. Fix failures within the authorized scope; do not push with applicable checks failing. ## Change guidance - Query changes often touch broker planning and server execution; verify both. @@ -171,15 +121,14 @@ should read kb/ directly. `review-concurrency-state`, `review-architecture`, `review-performance`, `review-correctness-nulls`, `review-testing`, `review-naming-api`, `review-process-scope`. -- `kb/agents/code-reviewer.md` — orchestrator that dispatches the eight review - skills in parallel, aggregates findings, and emits a consolidated severity- - ranked report. +- `kb/agents/code-reviewer.md` — review procedure that selects relevant domains, + delegates substantial independent checks, and verifies consolidated findings. - `kb/code-review-principles.md` — Pinot-specific review principles cited by id (e.g. `C2.4`, `C6.1`) from the review skills. -- `kb/CLAUDE.md` — kb/ authoring rules (one source of truth, terse, AI-optimized). +- `kb/claude.md` — kb/ authoring rules (one source of truth, terse, AI-optimized). **For non-Claude agents:** when a task matches a skill name (e.g. user asks for a pre-commit check, a benchmark comparison, a flaky-test investigation, or a code review), read the corresponding `kb/skills/.md` and follow its -procedure. For a full code review, read `kb/agents/code-reviewer.md` and run the -eight review skills as it describes. +procedure. For a full code review, follow `kb/agents/code-reviewer.md` and cover +the domains relevant to the diff; small changes can be reviewed directly. diff --git a/CLAUDE.md b/CLAUDE.md index 046bd537a099..6b1ad1c40250 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,64 +39,12 @@ Apache Pinot is a real-time distributed OLAP datastore for low-latency analytics | `pinot-integration-tests` | End-to-end validation suites | | `pinot-distribution` | Packaging artifacts | -## Plugin modules (`pinot-plugins`) -- **pinot-input-format**: input format plugin family. - - `pinot-arrow`: Apache Arrow input format support. - - `pinot-avro`: Avro input format support. - - `pinot-avro-base`: shared Avro utilities and base classes. - - `pinot-bson`: MongoDB BSON input format support. - - `pinot-clp-log`: CLP log input format support. - - `pinot-confluent-avro`: Confluent Schema Registry Avro input support. - - `pinot-confluent-json`: Confluent Schema Registry JSON input support. - - `pinot-confluent-protobuf`: Confluent Schema Registry Protobuf input support. - - `pinot-orc`: ORC input format support. - - `pinot-json`: JSON input format support. - - `pinot-parquet`: Parquet input format support. - - `pinot-csv`: CSV input format support. - - `pinot-thrift`: Thrift input format support. - - `pinot-protobuf`: Protobuf input format support. -- **pinot-file-system**: filesystem plugin family. - - `pinot-adls`: Azure Data Lake Storage (ADLS) filesystem support. - - `pinot-hdfs`: Hadoop HDFS filesystem support. - - `pinot-gcs`: Google Cloud Storage filesystem support. - - `pinot-s3`: Amazon S3 filesystem support. -- **pinot-batch-ingestion**: batch ingestion plugin family. - - `pinot-batch-ingestion-common`: shared batch ingestion APIs and utilities. - - `pinot-batch-ingestion-spark-base`: shared Spark ingestion base classes. - - `pinot-batch-ingestion-spark-3`: Spark 3 ingestion implementation. - - `pinot-batch-ingestion-hadoop`: Hadoop MapReduce ingestion implementation. - - `pinot-batch-ingestion-standalone`: standalone batch ingestion implementation. -- **pinot-stream-ingestion**: stream ingestion plugin family. - - `pinot-kafka-base`: shared Kafka ingestion base classes. - - `pinot-kafka-3.0`: Kafka 3.x ingestion implementation. - - `pinot-kafka-4.0`: Kafka 4.x ingestion implementation. - - `pinot-kinesis`: AWS Kinesis ingestion implementation. - - `pinot-pulsar`: Apache Pulsar ingestion implementation. -- **pinot-minion-tasks**: minion task plugin family. - - `pinot-minion-builtin-tasks`: built-in minion task implementations. -- **pinot-metrics**: metrics reporter plugin family. - - `pinot-dropwizard`: Dropwizard Metrics reporter implementation. - - `pinot-yammer`: Yammer Metrics reporter implementation. - - `pinot-compound-metrics`: compound metrics implementation. -- **pinot-segment-writer**: segment writer plugin family. - - `pinot-segment-writer-file-based`: file-based segment writer implementation. -- **pinot-segment-uploader**: segment uploader plugin family. - - `pinot-segment-uploader-default`: default segment uploader implementation. -- **pinot-environment**: environment provider plugin family. - - `pinot-azure`: Azure environment provider implementation. -- **pinot-timeseries-lang**: time series language plugin family. - - `pinot-timeseries-m3ql`: M3QL language plugin implementation. -- **assembly-descriptor**: Maven assembly descriptor for plugin packaging. +When locating a plugin implementation, consult [kb/plugin-modules.md](kb/plugin-modules.md). ## Build commands -- **JDK**: Use JDK 25+ for Pinot services and the default build; client and SPI artifacts still target Java 11 bytecode. -- **Default build**: `./mvnw clean install` -- **Fast dev build**: `./mvnw verify -Ppinot-fastdev` -- **Full binary/shaded build**: `./mvnw clean install -DskipTests -Pbin-dist -Pbuild-shaded-jar` -- **Build a module with deps**: `./mvnw -pl pinot-server -am test` -- **Single test**: `./mvnw -pl pinot-segment-local -Dtest=RangeIndexTest test` -- **Single integration test**: `./mvnw -pl pinot-integration-tests -am -Dtest=OfflineClusterIntegrationTest -Dsurefire.failIfNoSpecifiedTests=false test` -- **Quickstart (after build)**: `build/bin/quick-start-batch.sh` +Use [AGENTS.md](AGENTS.md#build-and-test) for the build JDK, artifact bytecode targets, and scoped build/test commands. +Use [kb/skills/run-test.md](kb/skills/run-test.md) for targeted tests and +[kb/skills/quickstart.md](kb/skills/quickstart.md) to start and verify a local cluster. ## Code style and formatting - Run `./mvnw spotless:apply` to auto-format code. @@ -107,7 +55,7 @@ Apache Pinot is a real-time distributed OLAP datastore for low-latency analytics ## Coding conventions - Add class-level Javadoc for new classes; describe behavior and thread-safety. -- Use Javadoc comments (`/** ... */` or `///` syntax); service code targets Java 25 by default. +- Use Javadoc syntax supported by the owning module's configured source release; see [AGENTS.md](AGENTS.md#coding-conventions-and-hygiene). - Keep Apache 2.0 license headers on all new source files. - Preserve backward compatibility across mixed-version broker/server/controller. - Prefer imports over fully qualified class names (e.g., use `import com.foo.Bar` and refer to `Bar`, not `com.foo.Bar` inline). @@ -127,32 +75,23 @@ Apache Pinot is a real-time distributed OLAP datastore for low-latency analytics - **Fix**: Omit the `Co-authored-by` line entirely when committing. ## Pre-commit checks -Before pushing a commit, run the following checks on the affected modules and fix any failures: -1. `./mvnw spotless:apply -pl ` — auto-format code. -2. `./mvnw checkstyle:check -pl ` — validate style conformance. -3. `./mvnw license:format -pl ` — add missing license headers to new files. -4. `./mvnw license:check -pl ` — verify all files have correct license headers. - -Do not push until all four checks pass cleanly. - -Additionally, run the compiler warning check and fix what you can: -5. `./mvnw test-compile -pl -am -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true` — review warnings (deprecation, unchecked, etc.) in your changed code and fix where possible. - -Claude Code users can invoke `/precommit` to automate all of the above. +Before pushing, follow [kb/skills/precommit.md](kb/skills/precommit.md) for affected-module formatting, license headers, +checkstyle, and license validation. That procedure defines applicability, execution order, evidence reuse, and when +compiler warning checks are needed. Fix failures within the authorized scope; do not push with applicable checks failing. +Claude Code users can invoke `/precommit`. ## Change guidance - **Query changes** often touch broker planning and server execution; verify both. - **Segment/index changes** usually live under `pinot-segment-local` and `pinot-segment-spi`. - **Config or API changes** should update relevant configs and docs where applicable. -## Mandatory code review - -After completing any coding task (bug fix, feature, refactor, etc.), you MUST run the `code-reviewer` agent before presenting the work as done. This is non-negotiable. +## Code review -- Pass ONLY the review scope and a one-line change description. Do NOT pass your analysis, reasoning, or opinions — the reviewer must judge the code independently. -- Example invocation: `"Review unstaged changes in pinot-broker. Change: added timeout to scatter-gather calls."` -- If the reviewer finds CRITICAL issues, fix them before proceeding. MAJOR issues should be fixed unless you have strong justification. MINOR issues are at your discretion. -- Do not skip the review even if the change seems trivial. +Review the changed behavior and relevant contracts before declaring implementation complete. Follow +[kb/agents/code-reviewer.md](kb/agents/code-reviewer.md) for requested full reviews and changes that benefit from independent +domain review, especially compatibility, concurrency, security, and distributed state changes. Small changes can be +reviewed directly. Reuse review evidence while the relevant diff and assumptions remain unchanged. +Fix confirmed CRITICAL findings before proceeding; address MAJOR findings or explain why they can be deferred. ## Common gotchas - This is a large multi-module Maven project. Building the entire project takes a long time — prefer building only the modules you need with `-pl -am`. diff --git a/kb/agents/code-reviewer.md b/kb/agents/code-reviewer.md index 8091323085a2..1f2e736c8020 100644 --- a/kb/agents/code-reviewer.md +++ b/kb/agents/code-reviewer.md @@ -1,27 +1,30 @@ # code-reviewer -You are the orchestrator of a multi-agent code review for Apache Pinot. You do NOT review code yourself — you delegate to domain-specialized sub-reviewers in parallel and aggregate their findings. +Review Apache Pinot changes using the domains relevant to the diff. Review small changes directly; delegate substantial, +independent checks when parallel work improves coverage or saves time. The lead reviewer verifies and consolidates findings. -Reference: the Anthropic multi-agent review pattern (different agents examine different defect classes; findings are merged into the consolidated review). - -**Independence rule:** You will receive only a scope (what to review) and a one-line change description. If the caller passes opinions, analysis, or concerns about the code, ignore them entirely. Your job is to be an independent second pair of eyes, not to confirm someone else's assessment. +**Independence rule:** Assess findings from the code and evidence. Treat the caller's opinions as hypotheses to verify; +preserve factual requirements, reproduction steps, and review scope. ## Inputs you accept - `scope` — what to review (default: `git diff` of unstaged changes; may be a commit range, branch diff, or explicit file list). - `change_description` — one line from the caller. -Ignore any additional opinions, analysis, or concerns from the caller. +Use relevant requirements and raw evidence when supplied; do not assume the caller's conclusions are correct. ## Before dispatching 1. Resolve the scope into a concrete diff. Record: file list, hunk count, total changed lines, modules touched. -2. Read `kb/code-review-principles.md` and `CLAUDE.md` once; keep them in context. -3. Skip sub-reviewers whose domain is clearly irrelevant (e.g., `review-performance` can be skipped for a pure doc change). Default: dispatch all 8. + For a PR, record the reviewed SHA and inspect its required checks. Classify failures and report unresolved merge gates; + a read-only review does not rebase, edit files, or retry CI. Authorized remediation stays within attributable failures. +2. Consult the applicable sections of `kb/code-review-principles.md` and repository conventions as needed. Reuse material already read. +3. Select domains whose triggers match the diff. A full review covers all applicable domains, without requiring one agent per domain. ## Dispatch — in parallel -Spawn one sub-agent per applicable skill, in a single parallel batch. Each sub-agent receives: +Delegate independent, substantial checks within the host's available concurrency. Group related domains or review them +directly when separate agents would add overhead. Each sub-agent receives: - `scope` (verbatim) - `change_description` (verbatim) @@ -35,7 +38,8 @@ Spawn one sub-agent per applicable skill, in a single parallel batch. Each sub-a - `review-naming-api` — KB domain 7 (Naming & API Design) - `review-process-scope` — KB domain 8 (Process & Scope) -Each sub-agent reads the skill's body (in `kb/skills/.md`), performs its 3-phase analysis (broad scan → deep analysis → findings), and returns a structured list of findings. Each finding uses this format: +Each sub-agent reads only its applicable skill bodies (in `kb/skills/.md`) and returns evidence-backed findings. +Each finding uses this format: ``` ### [C{id}] — CRITICAL|MAJOR|MINOR @@ -49,19 +53,14 @@ Use `[BUG]` for bugs not covered by a specific principle, `[CONV]` for CLAUDE.md ## Severity hierarchy -Classify each finding using the tiers defined in the principles doc: - -- **CRITICAL**: Must fix before merge. Data loss, corruption, silent wrong results, backward incompatibility, security, race conditions. -- **MAJOR**: Should fix. Strong justification needed to skip. Performance regressions, design violations, missing tests, wrong abstractions. -- **MINOR**: Improves quality. Acceptable to defer. Naming, style, idioms, process suggestions. - -Priority order when principles collide: Production Safety > Backward Compatibility > Correctness > State Management > Performance > Architecture > Testing > Naming > Process. +Use the severity definitions and priority order in `kb/code-review-principles.md` as the single source. Classify demonstrated +impact, not the matched pattern or missing process artifact. Unverified assumptions belong in coverage limits. ## Aggregate 1. Collect all findings into one list. -2. **De-duplicate** by the key `(principle_id || "BUG:"+one_line_problem, file, line_range_overlap)`. When two sub-reviewers flag the same issue, keep the one with the higher severity and append `also-flagged-by: <skill>` to the record. -3. **Resolve conflicts** — if two skills disagree on severity, take the higher tier and note the disagreement in a `notes` field so the human reviewer can weigh in. +2. **De-duplicate** by the key `(principle_id || "BUG:"+one_line_problem, file, line_range_overlap)`. Merge supporting evidence and append `also-flagged-by: <skill>` to the record. +3. **Resolve conflicts** — verify the trigger and impact against the code, correct unsupported findings or severity, and explain material changes. Report unresolved uncertainty explicitly. 4. **Sort** by severity (CRITICAL → MAJOR → MINOR), then by file, then by line. 5. **Cap noise** — if more than 15 MINOR findings accumulate, summarize them in one "Style / nits" section rather than listing each. @@ -70,12 +69,13 @@ Priority order when principles collide: Production Safety > Backward Compatibili Follow the review-delivery rules in `kb/code-review-principles.md`. In plain-text review surfaces, use the consolidated report below. -Start by listing what you're reviewing (files, diff summary, dispatched sub-reviewers). Then emit a consolidated report in this shape: +State the review scope and relevant coverage. For substantial reviews, use the report below; for small reviews, report +findings and validation limits concisely without empty sections. ``` ## Review scope - Files: N, Lines: +X/-Y, Modules: m1, m2 -- Sub-reviewers dispatched: 8 (or list if fewer) +- Domains reviewed: <list; note which were delegated> ## CRITICAL (must fix before merge) ### [<principle_id or BUG>] <title> — CRITICAL @@ -98,14 +98,13 @@ Start by listing what you're reviewing (files, diff summary, dispatched sub-revi - Any inter-skill disagreements flagged for the human reviewer ``` -If no issues are found across all dispatched sub-reviewers, confirm the code meets standards with a brief summary noting which domains were checked. +If no issues are found, say so briefly and note the domains checked and any material coverage limits. ## Discipline -- **Never add findings of your own.** You only aggregate. If you notice something the sub-reviewers missed, dispatch the relevant skill again; do not invent findings here. -- **Never downgrade severity.** If sub-reviewers say CRITICAL, the consolidated report says CRITICAL. +- **Verify findings.** The lead reviewer may add evidence-backed findings and correct severity without another dispatch. - **Trigger matching is mandatory.** Sub-reviewers only apply principles whose trigger conditions match the diff. A sub-reviewer that returns "no applicable principles in this domain" is a valid, reassuring result — record it in the summary. -- **Cite the most severe principle** when a finding matches multiple rules. +- **Cite the principle that best explains the defect** when several rules match; supporting citations do not raise severity. - **Severity accuracy is paramount.** A MINOR issue classified as CRITICAL erodes trust just as much as a missed CRITICAL. - **Quality over quantity.** A review with 2 real findings beats one with 10 marginal ones. - **If a sub-reviewer errors**, record the failure in the summary and fall back to reporting the surviving sub-reviewers' findings. Do not silently drop a domain. diff --git a/kb/claude.md b/kb/claude.md index a7b12f83a4d4..718ac3985b1c 100644 --- a/kb/claude.md +++ b/kb/claude.md @@ -21,5 +21,5 @@ ## Quality Bar - Accurate and current — verify before writing -- Actionable — specific commands, step-by-step procedures, validation steps +- Actionable — concrete commands and completion criteria; require ordered steps only when correctness depends on their order - Focused — operationally important details only, skip the obvious diff --git a/kb/code-review-principles.md b/kb/code-review-principles.md index e4699cef50b0..79c182cc04d1 100644 --- a/kb/code-review-principles.md +++ b/kb/code-review-principles.md @@ -3,10 +3,14 @@ Engineering principles governing Apache Pinot development, organized by severity within each domain. Severity definitions: -- **CRITICAL**: Must fix before merge. Data loss, corruption, silent wrong results, backward incompatibility, security issues, race conditions. -- **MAJOR**: Should fix. Strong justification needed to skip. Performance regressions, design violations, missing tests, wrong abstractions. +- **CRITICAL**: Must fix before merge. Confirmed risks of data loss, corruption, wrong results, broken supported compatibility, security violations, or concurrency failures affecting correctness or availability. +- **MAJOR**: Should fix. Demonstrated material performance regressions, design defects, or coverage gaps that leave important changed behavior unverified. - **MINOR**: Improves quality. Acceptable to defer. Naming, style, idioms, process suggestions. +These definitions are the single source for finding severity. Section labels, examples, and pattern matches guide +investigation; they are not findings. Assign severity from concrete impact and reachable failure conditions. Missing +documentation, tests, or benchmark evidence, process deviations, and size thresholds alone are not CRITICAL findings. + Priority order: Production Safety > Backward Compatibility > Correctness > State Management > Performance > Architecture > Testing > Naming > Process ## Review Delivery @@ -15,6 +19,10 @@ Priority order: Production Safety > Backward Compatibility > Correctness > State are unavailable, include exact file and line references. - Always include a concise review summary covering the scope, overall assessment, and finding counts by severity. - Keep the summary synthetic; do not repeat the full text of every inline finding. +- For review-only tasks, complete when relevant risks are assessed and actionable findings and material validation gaps + are reported. Use targeted read-only verification to support findings. Do not run tests, modify the checkout, or publish + findings solely to satisfy a checklist; those actions require authorization from the task. Continue implementation or + fixes when the task authorizes that work. --- @@ -97,7 +105,9 @@ if (effective == Enablement.ENABLE) { /* enable upsert */ } ### MAJOR **C1.6 — New features default to OFF** -Behavior-changing features default to disabled. Boolean flags should default to `false`. Name as `disableXXX` so default=false means enabled. +New behavior-changing features default to disabled: `enableXxx=false`, or `disableXxx=true` when using an established +disable-style key. Judge the effective behavior, not the literal boolean value. Preserve existing flag names and defaults +unless a default change is explicitly intended and validated. - Trigger: Any PR adding a new feature flag or boolean config **C1.7 — Config namespace consistency** @@ -169,26 +179,30 @@ if (!helixAdmin.setResourceIdealState(clusterName, tableName, idealState, expect ``` **C2.2 — Thread safety conservatism** -When uncertain about framework threading guarantees (e.g., gRPC StreamObserver), default to explicit synchronization (`volatile`, `synchronized`). Read upstream library source code to verify actual contracts. +Establish the threading contract and shared-state access before choosing synchronization. Check relevant framework +documentation or source when the contract is unclear. Add synchronization for identified concurrent access, not merely +because a callback or framework is involved. - Trigger: Any PR modifying shared mutable state or concurrent code ```java -// BAD: Assumes gRPC StreamObserver is thread-safe (it's NOT for onNext) +// BAD: Two application producers invoke this observer concurrently without protecting shared state class MyObserver implements StreamObserver<DataBlock> { private List<DataBlock> blocks = new ArrayList<>(); // unsynchronized - public void onNext(DataBlock block) { blocks.add(block); } // called from gRPC threads + public void onNext(DataBlock block) { blocks.add(block); } } ``` ```java -// GOOD: Explicit synchronization until threading contract is verified +// GOOD: Protect concurrent additions after confirming the two-producer threading contract class MyObserver implements StreamObserver<DataBlock> { private final List<DataBlock> blocks = Collections.synchronizedList(new ArrayList<>()); public void onNext(DataBlock block) { blocks.add(block); } } ``` -**C2.3 — Exhaustive race condition analysis for lock changes** -When moving from coarse-grained to fine-grained locking, enumerate ALL possible interleavings. Missing IS updates = MAJOR correctness issue. +**C2.3 — Verify affected invariants when changing locks** +Identify the shared state and invariants affected by the lock change. Analyze key competing operations and failure +interleavings, with focused tests where needed. Preserve coupled updates; exhaustive enumeration of every possible +thread schedule is not required. - Trigger: Any PR changing lock granularity or locking strategy ```java @@ -198,7 +212,7 @@ When moving from coarse-grained to fine-grained locking, enumerate ALL possible + // IdealState update now outside lock — another thread can see stale routing ``` ```java -// GOOD: Enumerate interleavings; keep coupled operations under same lock +// GOOD: Verify competing updates preserve the invariant; keep coupled operations under the same lock synchronized (_tableLocks.get(table)) { updateRoutingTable(table); updateIdealState(table); // must stay atomic with routing update @@ -243,7 +257,7 @@ synchronized (this) { ``` **C2.6 — Thread safety in shared observers must be proven correct** -Mutable state in shared gRPC observers is a MAJOR concern. +For observers whose callbacks can run concurrently, verify safe publication and protection of mutable state. - Trigger: Any PR modifying gRPC StreamObserver implementations or shared callback handlers ```java @@ -256,7 +270,7 @@ class BlockObserver implements StreamObserver<DataBlock> { ```java // GOOD: Use atomic or synchronize; document the threading model class BlockObserver implements StreamObserver<DataBlock> { - // gRPC calls onNext from transport threads — must be thread-safe + // Application producers can call onNext concurrently; protect the shared counter private final AtomicInteger blockCount = new AtomicInteger(0); public void onNext(DataBlock b) { blockCount.incrementAndGet(); processBlock(b); } } @@ -347,12 +361,14 @@ public BrokerResponse handleQuery(JsonNode query) { } ``` -**C3.3 — Centralize ZooKeeper writes in ZKOperator pattern** -Other classes pass metadata objects without writing directly. Separate in-memory mutation from ZK persistence for batching and version-aware writes. -- Trigger: Any PR writing to ZK outside the designated ZKOperator class +**C3.3 — Preserve the owning subsystem's ZooKeeper persistence boundary** +Within segment upload, preserve the established `ZKOperator` / `PinotHelixResourceManager` ownership boundary. Other +subsystems use their own persistence owner; do not route unrelated writes through the upload helper. Separate in-memory +mutation from persistence where needed for batching, version checks, and failure handling. +- Trigger: Any PR bypassing its subsystem's persistence owner or required consistency checks ```java -// BAD: ZK write scattered in a helper class — bypasses version control and batching +// BAD: Segment-upload helper bypasses its owner's version checks and batching class SegmentStatusUpdater { void markComplete(String segment) { SegmentZKMetadata meta = ZKMetadataProvider.getSegmentMetadata(store, segment); @@ -371,8 +387,10 @@ class SegmentStatusUpdater { } ``` -**C3.4 — New feature code paths must be completely separate from existing ones** -Ensure backward compatibility by not modifying shared paths. Scope engine-specific changes to the relevant engine. +**C3.4 — Isolate feature-specific semantics while preserving existing behavior** +Scope feature- or engine-specific behavior to its intended callers. Reuse or modify shared implementations when existing +contracts remain compatible, and verify affected existing callers. Separate implementations are not required solely +because a feature is new. - Trigger: Any PR adding a new feature that touches existing query/ingestion paths ```java @@ -420,7 +438,8 @@ All query option parsing in `QueryOptionsUtils`. Mechanism changes in central ut - Trigger: Any PR with utility logic duplicated across call sites **C3.8 — Module placement** -Shared context classes go in lowest feasible module. `pinot-spi` is for interfaces only, concrete implementations in runtime modules. +Shared context classes go in the lowest feasible module. `pinot-spi` contains public SPI contracts and their +dependency-light data/config types; service implementations belong in runtime modules. - Trigger: Any PR adding new classes — check module placement **C3.9 — Caller-side validation over internal flags** @@ -865,7 +884,10 @@ State this explicitly. When renaming types, update all corresponding variable na ### CRITICAL **C6.1 — CI must be green before merge** -Backward compatibility tests are non-negotiable. Fix linter failures even when PR is approved. +Inspect required checks for the exact reviewed SHA before claiming merge readiness. Classify failures as attributable to +the change, pre-existing, flaky, infrastructure-related, or unresolved; required checks that are pending or failing remain +merge gates. Read-only review reports those gates without rebasing, editing, or retrying CI. When remediation is authorized, +fix attributable failures within scope, including compatibility and lint failures, and verify checks on the resulting SHA. - Trigger: Any PR with failing CI checks ```java @@ -873,8 +895,9 @@ Backward compatibility tests are non-negotiable. Fix linter failures even when P // japicmp backward-compat test fails → merge → breaks downstream plugin builds ``` ```java -// GOOD: Fix the compat issue or update the baseline with justification -// If genuinely unrelated: rebase on latest master, verify CI green, then merge +// GOOD: Report the required compatibility failure on the reviewed SHA +// If remediation is authorized, fix the regression and verify required checks on the new SHA +// If unrelated, report the evidence and remaining merge gate without changing the review scope ``` **C6.2 — Tests must not be more resilient than production code** @@ -901,19 +924,24 @@ Don't add retries/sleeps that production doesn't have. Fix root causes of flakin ### MAJOR **C6.3 — Bug fixes require regression tests** -Tests that fail without the fix and pass with it. -- Trigger: Any bug fix PR without a corresponding test +For behavioral fixes, retain or extend tests that distinguish the broken behavior from the correction. Reuse existing +coverage when it proves the regression. When coverage is unclear, verify failure on the unfixed baseline and success +with the fix; equivalent verified evidence need not be repeated. Use the actual pre-fix base or revert only the fix in +an isolated comparison; `HEAD~1` is suitable only when it represents that pre-fix state. Report unavailable evidence. +- Trigger: Any behavioral bug fix PR without evidence that the regression is covered -**C6.4 — Integration tests exercise the full pipeline** -Test user-facing queries through rewrite+optimization chain, not internal APIs. -- Trigger: Any PR adding tests for query behavior +**C6.4 — Test at the smallest layer that proves the behavior** +Use focused tests for local behavior. When a change depends on parsing, rewriting, optimization, or cross-layer +interaction, test through the relevant query pipeline. Reuse existing tests and shared integration clusters. +- Trigger: Query behavior changes whose regression coverage may miss an affected pipeline stage or interaction **C6.5 — Tests must actually validate claimed behavior** -If tests pass with invalid credentials, the test suite has a gap. +Assertions must distinguish the claimed behavior from the relevant failure case. An authentication-success test that +also passes with invalid credentials has a gap; an explicit credential-rejection test should pass for that input. - Trigger: Any PR where test assertions may be vacuously true **C6.6 — Guard serialization format with tests** -Add round-trip tests for Jackson-annotated classes. +Verify existing round-trip and compatibility coverage for changed serialization contracts; add cases for uncovered behavior. - Trigger: Any PR modifying JSON-serialized config or metadata classes **C6.8 — New tests must be verified as stable before merge** @@ -1139,20 +1167,22 @@ condition ### CRITICAL **C8.1 — SPI changes must be flagged** -Explicitly tag all teams maintaining plugins for review. SPI blast radius extends beyond core. -- Trigger: Any PR modifying classes in `pinot-spi` module +Assess plugin-visible contract changes and identify affected plugins and relevant reviewers. Severity follows the actual +compatibility impact, not the module path. Tag or request review from others only when the user authorizes that external +communication; otherwise identify the required reviewers in the review summary. +- Trigger: Any PR changing a plugin-visible SPI signature, contract, or behavior ```java -// BAD: Changed SPI interface method without notifying plugin teams +// BAD: Adds a required SPI method without preserving existing plugin compatibility // pinot-spi/src/.../RecordReader.java — added required method + void seekToOffset(long offset); // all existing RecordReader plugins now fail to compile ``` ```java -// GOOD: Add default method for backward compat; flag plugin teams for review +// GOOD: Keep existing readers usable; the new optional capability is unsupported by default + default void seekToOffset(long offset) { + throw new UnsupportedOperationException("Not implemented"); + } -// PR description: "@kafka-plugin-team @kinesis-plugin-team — SPI change, please review" +// Review summary: identify affected reader plugins and the relevant maintainers for compatibility review ``` **C8.2 — Do not modify deprecated features with known security implications** @@ -1221,9 +1251,11 @@ Require failing tests or concrete bugs before accepting performance-degrading ch Not just the solution. Include context for reviewers and future readers. - Trigger: Any PR with insufficient description -**C8.14 — Explore alternative approaches in separate PRs** -Compare designs before committing. -- Trigger: Any PR where alternative designs were discussed but not prototyped +**C8.14 — Document consequential alternatives and trade-offs** +Explain relevant alternatives and why the chosen approach meets the requirements. Prototype only when a critical unknown +cannot be resolved from existing evidence and the experiment is within the authorized scope. Discussing an option does +not require implementing it or opening another PR. +- Trigger: Any consequential design decision whose rationale or critical feasibility assumptions remain unclear **C8.15 — Model features aligned with domain standards** Text search follows Lucene/OpenSearch DSL. SQL functions follow SQL standard semantics. diff --git a/kb/plugin-modules.md b/kb/plugin-modules.md new file mode 100644 index 000000000000..13351226ef07 --- /dev/null +++ b/kb/plugin-modules.md @@ -0,0 +1,69 @@ +<!-- + + 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. + +--> +# Pinot plugin modules + +- pinot-input-format: input format plugin family. + - pinot-arrow: Apache Arrow input format support. + - pinot-avro: Avro input format support. + - pinot-avro-base: shared Avro utilities and base classes. + - pinot-bson: MongoDB BSON input format support. + - pinot-clp-log: CLP log input format support. + - pinot-confluent-avro: Confluent Schema Registry Avro input support. + - pinot-confluent-json: Confluent Schema Registry JSON input support. + - pinot-confluent-protobuf: Confluent Schema Registry Protobuf input support. + - pinot-orc: ORC input format support. + - pinot-json: JSON input format support. + - pinot-parquet: Parquet input format support. + - pinot-csv: CSV input format support. + - pinot-thrift: Thrift input format support. + - pinot-protobuf: Protobuf input format support. +- pinot-file-system: filesystem plugin family. + - pinot-adls: Azure Data Lake Storage (ADLS) filesystem support. + - pinot-hdfs: Hadoop HDFS filesystem support. + - pinot-gcs: Google Cloud Storage filesystem support. + - pinot-s3: Amazon S3 filesystem support. +- pinot-batch-ingestion: batch ingestion plugin family. + - pinot-batch-ingestion-common: shared batch ingestion APIs and utilities. + - pinot-batch-ingestion-spark-base: shared Spark ingestion base classes. + - pinot-batch-ingestion-spark-3: Spark 3 ingestion implementation. + - pinot-batch-ingestion-hadoop: Hadoop MapReduce ingestion implementation. + - pinot-batch-ingestion-standalone: standalone batch ingestion implementation. +- pinot-stream-ingestion: stream ingestion plugin family. + - pinot-kafka-base: shared Kafka ingestion base classes. + - pinot-kafka-3.0: Kafka 3.x ingestion implementation. + - pinot-kafka-4.0: Kafka 4.x ingestion implementation. + - pinot-kinesis: AWS Kinesis ingestion implementation. + - pinot-pulsar: Apache Pulsar ingestion implementation. +- pinot-minion-tasks: minion task plugin family. + - pinot-minion-builtin-tasks: built-in minion task implementations. +- pinot-metrics: metrics reporter plugin family. + - pinot-dropwizard: Dropwizard Metrics reporter implementation. + - pinot-yammer: Yammer Metrics reporter implementation. + - pinot-compound-metrics: compound metrics implementation. +- pinot-segment-writer: segment writer plugin family. + - pinot-segment-writer-file-based: file-based segment writer implementation. +- pinot-segment-uploader: segment uploader plugin family. + - pinot-segment-uploader-default: default segment uploader implementation. +- pinot-environment: environment provider plugin family. + - pinot-azure: Azure environment provider implementation. +- pinot-timeseries-lang: time series language plugin family. + - pinot-timeseries-m3ql: M3QL language plugin implementation. +- assembly-descriptor: Maven assembly descriptor for plugin packaging. diff --git a/kb/skills/README.md b/kb/skills/README.md index d171e51e25e3..76ed3f842546 100644 --- a/kb/skills/README.md +++ b/kb/skills/README.md @@ -8,10 +8,10 @@ Claude Code additionally exposes each skill as a slash command (`/<skill-name>`) | Skill | Purpose | Rough time | |---|---|---| -| [`precommit`](precommit.md) | Run the mandatory pre-commit checks (`spotless:apply`, `license:format`, `checkstyle:check`, `license:check`) plus compiler warning checks (`-Xlint:all`) on only the modules touched by the diff. | 30–120s warm, up to 5min cold | -| [`run-test <Class>`](run-test.md) | Resolve a test class name to its module and run the single-test Maven invocation. Auto-adds integration-test flags. | 30s–15min depending on test | -| [`quickstart [mode]`](quickstart.md) | Launch a local Pinot quickstart cluster (`batch`, `hybrid`, `streaming`, `upsert-streaming`, `auth`, …) in the background. | ~30s to ready | -| [`bench-compare <Benchmark> [<ref>]`](bench-compare.md) | Run a `pinot-perf` JMH benchmark against a baseline ref and the current tree and diff the JMH tables. Uses a git worktree. | 10min – days (see below) | +| [`precommit`](precommit.md) | Validate the four required checks on affected modules; run compiler warning checks for Java or build changes when current equivalent evidence is missing. | 30–120s warm, up to 5min cold | +| [`run-test <Class>`](run-test.md) | Resolve a test class name, verify dependency availability, and run the targeted Maven test. | 30s–15min depending on test | +| [`quickstart [mode]`](quickstart.md) | Build the required artifacts, launch a local Pinot quickstart, and verify readiness. | Depends on mode and build state | +| [`bench-compare <Benchmark> [<ref>]`](bench-compare.md) | Run a `pinot-perf` JMH benchmark against a baseline ref and the current tree and diff the JMH tables. Uses a git worktree. | Budget set before running | | [`flaky-analyze <TestClass>`](flaky-analyze.md) | Pull recent CI failures for a test class, cluster by stack trace, propose a root-cause hypothesis. Investigation only. | 1–10min per 20 runs scanned | Review skills (consumed by the [`code-reviewer`](../agents/code-reviewer.md) agent): @@ -31,161 +31,38 @@ Review skills (consumed by the [`code-reviewer`](../agents/code-reviewer.md) age ## `precommit` -**What it does.** Detects modified files (staged + unstaged + new untracked `.java` / `.xml` / etc.), maps them to their owning Maven modules by walking up to the nearest `pom.xml`, then runs five checks in order on only those modules: formatting, license headers, style validation, and compiler warnings (via `-Xlint:all`). - -**The five checks:** -- `spotless:apply` — imports only (order + unused removal). Does **not** fix whitespace, indentation, or braces. Pinot's config is deliberately narrow; see the `spotless-maven-plugin` block in the root `pom.xml`. -- `license:format` — inserts the ASF header into new files that don't have it. Governed by `HEADER` at repo root. -- `checkstyle:check` — `config/checkstyle.xml`. Top offenders: `LineLength` (120), `AvoidStarImport`, `AvoidStaticImport`, `HideUtilityClassConstructor`, `NeedBraces`. -- `license:check` — final gate confirming every touched file has a header. -- `test-compile -Xlint:all` — compiles both `src/main/` and `src/test/` with all compiler warnings enabled (deprecation, unchecked casts, raw types, fallthrough, etc.). Does not use `clean` — incremental compilation still emits warnings for the entire module, and per-line filtering handles pre-existing warnings. Using `clean` would break modules with generated sources (e.g., JavaCC in `pinot-common`). Warnings are filtered to only lines added in the diff (not just by file). Uses `-am` because compilation needs upstream deps. - -**Example scenarios:** - -- **Clean tree** → prints `No changed Java/XML files — nothing to do.` and exits. Safe to run anytime. -- **Unused import** → `spotless:check` fails with a coloured diff; `spotless:apply` removes it. *Note:* removal leaves a cosmetic double blank line where the import used to be. Checkstyle does not flag this; the user can clean it up manually if desired. -- **Missing ASF header on new file** → `license:check` fails with `Some files do not have the expected license header`; `license:format` inserts the ASF header at the top of the file. No manual intervention needed. -- **Line >120 chars** → `checkstyle:check` fails with `[WARNING] src/.../File.java:[NN] (sizes) LineLength: Line is longer than 120 characters (found NNN).` Skill reports file:line; user must fix manually. -- **Deprecated API usage** → `test-compile -Xlint:all` emits `[WARNING] File.java:[line,col] method X has been deprecated`. Skill reports the warning and the non-deprecated replacement. Prefer the replacement; suppress with `@SuppressWarnings` only with a justifying comment. -- **Multi-module diff** → modules are joined with commas into one `-pl <m1>,<m2>,...` argument. All goals run once per step, scoped to the union. -- **Nested plugin module** (e.g. `pinot-plugins/pinot-input-format/pinot-csv/...`) → skill walks up past the `pinot-input-format/pom.xml` aggregator (it has no `src/`) and stops at `pinot-csv/pom.xml`. That's the correct module. - -**Usage:** -- `/precommit` — default, scoped to diff vs. HEAD + untracked. -- `/precommit staged` — staged changes only. -- `/precommit branch` — diff vs. `upstream/master` (useful before a PR). -- `/precommit all` — run on the entire repo; slow (several minutes). - -**Known quirks:** -- Steps 1–4 don't need `-am`. Only step 5 (compile) uses `-am` because javac needs upstream jars on the classpath. -- Spotless sometimes reformats files you hadn't touched if they were non-compliant to begin with. Review the auto-fix diff before staging. -- Violations in `pinot-controller/src/main/resources/` (the React UI) are not handled by the Maven plugins — skip that tree. -- Compiler warnings are filtered to added lines in the diff only — pre-existing warnings, even in files you touched, are not reported. +See [precommit.md](precommit.md) for scope selection, required checks, compiler evidence, authorized fixes, and staging. Reuse valid results; rerun checks invalidated by subsequent changes. --- ## `run-test` -**What it does.** Given a test class name (or `Class#method`), finds the source file via glob, walks up to the owning module, and builds the correct `./mvnw -pl <module> -am -Dtest=<Class>[#<method>] -Dsurefire.failIfNoSpecifiedTests=false test` command. - -**Why `-Dsurefire.failIfNoSpecifiedTests=false` is always needed:** with `-am`, Maven builds upstream modules and runs Surefire in each one. Upstream modules don't have the target test, so Surefire's default behaviour (fail when the `-Dtest` filter matches nothing) kills the build at the first upstream module. The flag makes "no tests matched in this module" a no-op and lets the build progress to the module that actually contains the test. - -**Integration test heuristics** (used only to warn the user about expected runtime, not to change the command): path contains `pinot-integration-tests`, OR filename ends with `IntegrationTest.java` / `IT.java` / `ClusterTest.java` / `EndToEndTest.java`, OR the module is `pinot-integration-tests` / `pinot-compatibility-verifier`. - -**Example scenarios:** - -- **Unit test** → `/run-test BigDecimalUtilsTest` → resolves to `pinot-spi/src/test/java/.../BigDecimalUtilsTest.java` → runs `./mvnw -pl pinot-spi -am -Dtest=BigDecimalUtilsTest test`. Verified: 5 tests pass in ~6s after the dependency build. -- **Integration test** → `/run-test OfflineClusterIntegrationTest` → path `pinot-integration-tests/...` triggers integration detection → adds `-Dsurefire.failIfNoSpecifiedTests=false`. Runs for 10–20 minutes depending on the test. -- **Method selector** → `/run-test BigDecimalUtilsTest#testRoundTrip` → Maven's `-Dtest=Class#method` form. -- **Ambiguous name** → `/run-test AggregationFunctionColumnPairTest` → matches two files in `pinot-segment-spi` (`.../misc/` and `.../index/startree/`). Skill lists both with their package paths and asks the user to pick. Does not guess. -- **Not found** → `/run-test TypoTest` → glob returns zero hits → skill reports and stops. -- **Abstract base class** → warns that the class has no `@Test` methods and suggests concrete subclasses via grep. - -**Known quirks:** -- First run builds all upstream modules via `-am`, which can be 5–15 minutes on a cold tree. Subsequent runs against the same module skip rebuilds. -- Pinot uses TestNG; Surefire's `-Dtest=Class#method` syntax still works. -- Integration tests spin up embedded Helix/ZK/Kafka and bind to localhost ports. Don't run two at once. +See [run-test.md](run-test.md) for selectors, dependency requirements, asynchronous execution, and verification that the requested test ran. --- ## `quickstart` -**What it does.** Finds `quick-start-<mode>.sh` in `build/bin/` (produced by `-Pbin-dist`) or `pinot-tools/target/pinot-tools-pkg/bin/` (produced by a plain `pinot-tools` build), launches it in the background, and reports the controller URL. - -**Available modes:** `batch`, `hybrid`, `streaming`, `upsert-streaming`, `partial-upsert-streaming`, `json-index-batch`, `json-index-streaming`, `complex-type-handling-offline`, `complex-type-handling-streaming`, `auth`, `auth-zk`. Listed from the `pinot-tools` package. - -**Example scenarios:** - -- **Scripts already built** → runs `quick-start-batch.sh` in the background. After ~30s the controller is up at `http://localhost:9000`. Verified with `curl http://localhost:9000/tables` (returns `{"tables":[...]}`) and an SQL query (`SELECT COUNT(*) FROM airlineStats` → 9746 rows in 33ms). -- **No build yet** → skill offers two options: full `-Pbin-dist -Pbuild-shaded-jar` (~10min) or `pinot-tools` only (~3min). -- **Invalid mode** → skill lists the available scripts via `ls`. -- **Port 9000 already taken** → cluster start fails with `BindException`. Skill reports and asks whether to kill the existing process. - -**To stop the cluster:** kill the background shell launched by the skill (the skill records its id), or `pkill -f QuickStart`. - -**Known quirks:** -- All quickstart variants bind the same ports (9000 controller, 8000 broker, 7050/8098/8099 server). Only one can run at a time. -- The `auth` and `auth-zk` quickstarts use credentials `admin / verysecret`. -- Quickstarts run with embedded ZK + server + broker + controller in one JVM. That JVM is not small (~4GB heap) — expect to need a decently-sized machine. +See [quickstart.md](quickstart.md) for mode selection, prerequisite builds, process ownership, readiness checks, and stopping the cluster. --- ## `bench-compare` -**What it does.** Runs the same JMH benchmark twice — once against a baseline ref in a temporary git worktree, once against the current tree — and diffs the JMH output tables. - -**Time reality.** This is the slowest skill and it is not subtle. Pinot's default JMH config is **8 warmup × 60s + 8 measurement × 60s × 5 forks per `@Benchmark` method per parameter combination**. A single method with a few parameters can be a multi-hour run; the full default on something like `BenchmarkDictionary` estimated *7 days* in one test. The skill refuses to run without either explicit `--args` reducing iteration counts, or the user confirming they want the full default. - -**A reasonable first pass** for a single-method benchmark: `--args "-wi 1 -i 2 -f 1 -r 5s -w 5s"`. That's 1 warmup × 5s + 2 measurement × 5s × 1 fork ≈ 20s per method post-setup. Setup itself (the `@Setup` phase, which often builds Pinot segments) can still take 1–10 minutes for benchmarks that construct real tables. - -**Invocation style** (from validation: skill always uses this rather than the generated `.sh`): -``` -java -Xms4G -Xmx8G -cp '<tree>/pinot-perf/target/pinot-perf-pkg/lib/*' \ - org.openjdk.jmh.Main 'org.apache.pinot.perf.<BenchmarkClass>' \ - -wi 1 -i 2 -f 1 -r 5s -w 5s \ - -jvmArgsAppend='<full add-opens/add-exports set from pinot_tests.yml>' -``` - -Why `org.openjdk.jmh.Main` instead of the per-benchmark `pinot-<Class>.sh`: -- Benchmark classes have a custom `main()` that builds `OptionsBuilder` directly and ignores CLI args. Going through `org.openjdk.jmh.Main` bypasses it so flags like `-wi`, `-i`, `-r`, `-w`, and crucially `-jvmArgsAppend` actually take effect. -- The generated script hard-codes `-Xms24G -Xmx24G` which OOMs any laptop with less than ~32GB. -- The `--add-opens` / `--add-exports` flags are mandatory for any benchmark that extends `BaseClusterIntegrationTest` on JDK 21 — without them ZK startup dies with `InaccessibleObjectException`. The canonical set lives in `.github/workflows/pinot_tests.yml`. - -**Example scenarios:** - -- **Typical run** → `/bench-compare BenchmarkDictionary --args "-wi 1 -i 2 -f 1 -r 5s -w 5s"` — worktree at `/tmp/pinot-bench-baseline`, builds `pinot-perf` there, runs via `org.openjdk.jmh.Main`, diffs the JMH tables. -- **Cluster-backed benchmark** (e.g. `BenchmarkEquiJoin`, extends `BaseClusterIntegrationTest`) → skill automatically includes the full JDK-21 add-opens set in `-jvmArgsAppend`. -- **Explicit baseline** → `/bench-compare BenchmarkDictionary release-1.5.0` → same flow against a tagged release. -- **Vector benchmark** → `/bench-compare BenchmarkVectorIndex` → uses the `exec:java` form from `pinot-perf/README.md`; it has its own CLI conventions. - -**Known quirks:** - -- **Stale-jar trap (the real failure mode).** If the current tree's `pinot-perf/target/pinot-perf-pkg/lib/` was built from a previous ref that pulled in different versions of a transitive dep (e.g. `zookeeper-3.9.4.jar` + `zookeeper-3.9.5.jar`), the classpath glob loads both and you get `NoSuchMethodError` at runtime. Pinot/Helix often swallows this as `ZkTimeoutException: Unable to connect to zookeeper server within timeout: 1000`, which looks like an infrastructure/timing issue but is actually a classpath bug. The skill defends against this by always using `./mvnw -pl pinot-perf clean package -DskipTests -am` on the current tree (the worktree is a fresh checkout so baseline is immune). **If you see `ZkTimeoutException` in a second run, don't tune timeouts — check `lib/` for version duplicates.** -- JMH's `-l` flag doesn't help — Pinot benchmark classes have custom `main()` methods that ignore CLI args. There is no fast sanity check; the first real run is also the first verification. -- The generated `pinot-<Class>.sh` scripts hard-code `-Xms24G -Xmx24G`. Avoid them; use the `java -cp 'lib/*'` form with your own `-Xmx`. -- Only ~21 of ~60 benchmark classes are configured as appassembler programs — not every benchmark has a `.sh`. Direct `java -cp` works for all of them. -- Worktrees require a clean `.git`. Abort if rebase/merge is in progress. -- Benchmarks fork their own JVMs; don't be surprised by multiple `java` processes during the run. +See [bench-compare.md](bench-compare.md) for baseline isolation, JMH invocation, dependency pitfalls, run budgets, and retained results. Explicit bounded arguments or an existing budget do not require repeated confirmation. --- ## `flaky-analyze` -**What it does.** Queries GitHub Actions via `gh` CLI for recent failing runs of `Pinot Tests`, identifies the failing jobs, downloads the logs, greps for **real** Surefire/TestNG failure markers, clusters by stack trace, and reports a hypothesis. - -**Investigation only.** This skill never proposes a fix — the goal is to help the user decide whether the failure is a real bug, a race, or infrastructure noise. Principle C6.2 in `kb/code-review-principles.md` is explicit: the fix is never "add retries". - -**The right grep patterns** (documented after burning the wrong ones in testing): -- `\[ERROR\] Tests run: \d+, Failures: [1-9]` — a Surefire class summary with failures. The FQN follows `-- in `. -- `<<< FAILURE!` / `<<< ERROR!` — marker following a failed assertion. Line above has the method. -- `##\[error\]Process completed with exit code` — GitHub Actions' process-level marker. -- `BUILD FAILURE` — Maven's non-zero exit marker. - -Do **not** grep for bare `ERROR` / `FAILED` / `Exception` — Pinot's integration tests log those constantly at runtime (Helix rebalancer, Kafka consumer setup, etc.) and you will drown in noise. - -**Example scenarios:** - -- **Test is genuinely flaky on master** → skill finds multiple failing runs with the same stack trace → hypothesis: likely real race. Suggests specific source file and line, cites C2.x principles. -- **Test only fails on one matrix combination** (e.g. JDK 21 integration set 1) → likely env-specific → suggests checking JDK-specific behaviour. -- **Failing job has no Surefire markers** → infrastructure failure (timeout, OOM, runner cancel). Skill classifies as non-test-level and moves on. -- **Test name never appears** → either the test isn't actually failing (wrong search term) or GitHub's log retention has aged out the runs. Skill reports which and stops. - -**Usage:** -- `/flaky-analyze RangeIndexTest` — last 20 failing runs of `Pinot Tests`. -- `/flaky-analyze RangeIndexTest 50` — last 50. -- `/flaky-analyze RangeIndexTest --pr 18267` — only that PR's runs. - -**Known quirks:** -- `gh run view --log-failed` is unreliable here — it returns only the steps marked failed, which for Pinot's "Integration Test" step is often just runner init lines. Always use `--log` + grep. -- `gh run view --log` can return tens of MB per job. Cap runs scanned at 50 unless the user asks for more; fetch in parallel where possible. -- GitHub retains Actions logs for 90 days by default. Older flakes require a different data source (e.g. the `surefire-reports-*` artifacts uploaded on master runs). +See [flaky-analyze.md](flaky-analyze.md) for GitHub Actions queries, failure markers, source inspection, and the total run budget. Investigation remains report-only. --- ## Related configuration -- [`../agents/code-reviewer.md`](../agents/code-reviewer.md) — independent code-review agent that reads `kb/code-review-principles.md` and dispatches the eight `review-*` skills above. -- [`../code-review-principles.md`](../code-review-principles.md) — 163 Pinot-specific review principles; the review skills cite these by `C<chapter>.<id>`. +- [`../agents/code-reviewer.md`](../agents/code-reviewer.md) — code-review procedure that selects relevant review domains and consolidates findings. +- [`../code-review-principles.md`](../code-review-principles.md) — Pinot-specific review principles; the review skills cite these by `C<chapter>.<id>`. - [`../../CLAUDE.md`](../../CLAUDE.md) — project-wide instructions consumed by Claude Code. - [`../../AGENTS.md`](../../AGENTS.md) — general agent guidance (cross-tool). - [`../../.github/copilot-instructions.md`](../../.github/copilot-instructions.md) — overlapping guidance for GitHub Copilot / Cursor. @@ -193,8 +70,8 @@ Do **not** grep for bare `ERROR` / `FAILED` / `Exception` — Pinot's integratio ## Adding a new skill 1. Create the procedural body at `kb/skills/<skill-name>.md` — this is the source of truth, readable by any agent. -2. Write the procedure as numbered steps. Prefer concrete commands over prose. Keep it self-contained: a non-Claude agent reading this file should be able to follow it without additional context. -3. List the skill in the table above and add a detail section below. +2. Describe the task's trigger, outcome, relevant constraints, and concrete commands. Require ordered steps only where correctness depends on their order. Link to shared guidance rather than duplicating it. +3. Add the skill to the table above. Keep operational instructions in the procedure file; link rather than duplicate them. 4. (Optional, Claude Code only) Add a thin entry-point at `.claude/skills/<skill-name>/SKILL.md` containing YAML frontmatter (`name`, `description`, ASF header inside the frontmatter as `#` comments) and a one-line body pointing back to `kb/skills/<skill-name>.md`. This makes the skill invocable as `/<skill-name>` in Claude Code. Skills should be narrow, fast to read, and composable. A skill that "runs X and then does a code review" probably belongs as two separate skills chained by the user. @@ -204,4 +81,4 @@ Skills should be narrow, fast to read, and composable. A skill that "runs X and - **Claude Code skill isn't invoked when expected.** Claude may not have loaded `.claude/skills/` in its session context. In a new session, ask Claude to list available skills, or re-type the slash command explicitly. - **Maven wrapper not found.** All skills assume the repo root has `./mvnw`. If you're invoking from a subdirectory, ask the agent to `cd` first, or run from the repo root. - **`gh` not authenticated.** `flaky-analyze` requires `gh auth status` to succeed against github.com. Run `gh auth login` once. -- **Worktree errors in `bench-compare`.** Check that `/tmp/pinot-bench-baseline` isn't a leftover from a previous aborted run — if so, `git worktree remove --force` it and retry. +- **Worktree errors in `bench-compare`.** Follow [bench-compare.md](bench-compare.md) for unique worktree paths and ownership checks. Preserve an existing worktree whose ownership or changes are unknown. diff --git a/kb/skills/bench-compare.md b/kb/skills/bench-compare.md index 1f28c0e53593..9e7763354bc3 100644 --- a/kb/skills/bench-compare.md +++ b/kb/skills/bench-compare.md @@ -5,9 +5,9 @@ Purpose: when a change claims a performance impact (principle C6.7 — "performa Usage: - `/bench-compare BenchmarkDictionary` — compares current working tree vs. `merge-base HEAD upstream/master` (falls back to `origin/master` if upstream missing). - `/bench-compare BenchmarkDictionary <baseline-ref>` — compare against an explicit ref (commit, tag, branch). -- `/bench-compare BenchmarkDictionary --args "-wi 1 -i 2 -f 1 -r 5s -w 5s"` — pass extra JMH args. **Always use short warmup/iteration flags for a first pass**; defaults run for hours or days. +- `/bench-compare BenchmarkDictionary --args "-wi 1 -i 2 -f 1 -r 5s -w 5s"` — pass extra JMH args. Use short warmup/iteration flags for an exploratory first pass unless the user authorized a longer run; defaults can run for hours or days. -**Time expectations.** Pinot benchmarks are not quick. Default JMH config in `pinot-perf` is 8 warmup × 60s + 8 measurement × 60s × 5 forks per parameter combination — a single benchmark method's `@Benchmark` can report an ETA of multiple days. The skill will refuse to run without either: (a) explicit `--args` that reduce warmup/iteration counts, or (b) the user confirming they really do want the full default run. +**Time expectations.** Pinot benchmarks are not quick. Default JMH config in `pinot-perf` is 8 warmup × 60s + 8 measurement × 60s × 5 forks per parameter combination — a single benchmark method's `@Benchmark` can report an ETA of multiple days. Proceed with explicit bounded arguments or an already authorized time budget. Ask before an unbounded run or extending that budget; do not ask for the same authorization again. ## Procedure @@ -15,52 +15,80 @@ Usage: 2. **Resolve the baseline ref.** - Default: `git merge-base HEAD upstream/master`. If the `upstream` remote isn't defined, fall back to `origin/master`. If neither resolves, ask the user for an explicit ref. - - If the user passed a ref, validate it with `git rev-parse --verify <ref>`. - -3. **Prepare output directory.** `mkdir -p .bench-compare/` and append it to the repo's `.gitignore` if not already there. Produce two files: `baseline-<short-sha>.txt` and `current-<short-sha-or-WIP>.txt`. - -4. **Warn and confirm.** Benchmarks take real time. Inspect `--args` — if the user hasn't passed iteration controls, warn that the default suite can take hours to days and suggest a starter like `-wi 1 -i 2 -f 1 -r 5s -w 5s`. Print an estimate of the pair of runs (rough: a 5s-warmup × 5s-measurement × 1 fork run takes ~30–120s per `@Benchmark` method after the Pinot-side `@Setup` completes; `@Setup` alone can run for 1–10 minutes for benchmarks that build segments). Ask the user to confirm. - -5. **Build pinot-perf in a baseline worktree.** This avoids touching the working tree: + - If the user passed a ref, resolve it to a commit with `git rev-parse --verify '<ref>^{commit}'`. Record the resolved baseline SHA so both the build and report use the same commit. + +3. **Prepare a unique task directory outside the checkout.** Keep results separate from the temporary baseline worktree; do not modify the repository's `.gitignore` for local benchmark artifacts. The following examples share these variables; replace `<baseline-ref>` with the ref resolved in step 2: + ```sh + bench_repo=$(git rev-parse --show-toplevel) || exit 1 + bench_baseline_sha=$(git rev-parse --verify '<baseline-ref>^{commit}') || exit 1 + bench_run_dir=$(mktemp -d "${TMPDIR:-/tmp}/pinot-bench.XXXXXX") || exit 1 + bench_worktree="$bench_run_dir/baseline" + bench_results_dir="$bench_run_dir/results" + bench_worktree_created=false + mkdir "$bench_results_dir" || exit 1 ``` - git worktree add /tmp/pinot-bench-baseline <baseline-ref> - (cd /tmp/pinot-bench-baseline && ./mvnw -pl pinot-perf -am package -DskipTests) + Retain build logs, benchmark output, commands, resolved SHAs, and the scope of any uncommitted changes in the results directory. Report its absolute path; cleanup in step 8 removes only the owned baseline worktree. + +4. **Check the run budget.** Inspect the selected methods, parameter combinations, forks, warmup, measurement, and setup cost; estimate both builds and both runs. If explicit short arguments or an existing budget cover the work, state the estimate and proceed. With only a time budget, choose bounded arguments that fit it. Otherwise suggest a starter like `-wi 1 -i 2 -f 1 -r 5s -w 5s` and ask for the missing budget. Setup can take 1–10 minutes for benchmarks that build segments. If the estimate or observed runtime exceeds the authorized budget, preserve partial output and ask before extending the run. + +5. **Build pinot-perf in the baseline worktree.** Record ownership only after this task successfully creates it. Stop dependent steps if creation fails; do not reuse or delete an existing path: + ```sh + if git -C "$bench_repo" worktree add --detach "$bench_worktree" "$bench_baseline_sha"; then + bench_worktree_created=true + else + exit 1 + fi + (cd "$bench_worktree" && ./mvnw -pl pinot-perf -am package -DskipTests) \ + > "$bench_results_dir/baseline-build.txt" 2>&1 ``` The package goal produces the jars, an appassembler-generated launcher (for ~21 blessed benchmark classes) at `pinot-perf/target/pinot-perf-pkg/bin/pinot-<BenchmarkClass>.sh`, and a fat `lib/` directory. -6. **Run the baseline benchmark.** Two invocation styles, in order of preference: +6. **Verify selection and run the baseline benchmark.** Before starting measurements, list the selected methods and parameters without invoking their setup: + + ```sh + java -cp "$bench_worktree/pinot-perf/target/pinot-perf-pkg/lib/*" \ + org.openjdk.jmh.Main -lp 'org.apache.pinot.perf.<BenchmarkClass>' + ``` + Use `-l` instead of `-lp` when only method names are needed. Check the list against the budget from step 4; narrow the selector or parameters when needed. Do not invoke the benchmark's custom `main()` for discovery. **Preferred — always use JMH's own Main class:** ``` - java -Xms4G -Xmx8G -cp '/tmp/pinot-bench-baseline/pinot-perf/target/pinot-perf-pkg/lib/*' \ + (cd "$bench_worktree" && java -Xms4G -Xmx8G -cp "$bench_worktree/pinot-perf/target/pinot-perf-pkg/lib/*" \ org.openjdk.jmh.Main 'org.apache.pinot.perf.<BenchmarkClass>' \ -wi 1 -i 2 -f 1 -r 5s -w 5s \ - -jvmArgsAppend='-XX:+IgnoreUnrecognizedVMOptions --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/sun.nio.ch=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/jdk.internal.misc=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED -Dio.netty.tryReflectionSetAccessible=true' \ - > .bench-compare/baseline-<short-sha>.txt 2>&1 + -jvmArgsAppend='-XX:+IgnoreUnrecognizedVMOptions --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/sun.nio.ch=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/jdk.internal.misc=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED -Dio.netty.tryReflectionSetAccessible=true') \ + > "$bench_results_dir/baseline.txt" 2>&1 ``` Why not use the generated `pinot-<BenchmarkClass>.sh`? - It hard-codes `-Xms24G -Xmx24G` — OOMs on <32GB machines. - The benchmark's own `main()` (which the script invokes) typically constructs `OptionsBuilder` directly and **ignores CLI args**, so you can't override warmup/iterations or pass `-jvmArgsAppend`. Going through `org.openjdk.jmh.Main` bypasses the custom main and gets you JMH's standard CLI. - - The `--add-opens`/`--add-exports` flags are mandatory for any benchmark that extends `BaseClusterIntegrationTest` (i.e., spins up a Pinot cluster) on JDK 21 — without them, ZK startup fails with `InaccessibleObjectException` wrapped as `ExceptionInInitializerError`. + - Cluster-backed benchmarks can require `--add-opens`/`--add-exports` on the current JDK. Use the required flags consistently for both versions; diagnose module-access failures from their underlying exception. For the vector suite (`BenchmarkVectorIndex`) use the `exec:java` form from `pinot-perf/README.md`; it has its own quirks. -7. **Build and run the current tree.** Two mandatory gotchas: +7. **Build and run the current tree.** Verify dependency and packaging state before measuring: - - **Always clean first:** `./mvnw -pl pinot-perf clean package -DskipTests` (note the `clean`, no `-am` — see next bullet). If `pinot-perf/target/pinot-perf-pkg/` already exists from a prior build on a different branch/ref, incremental `package` leaves stale third-party jars in `lib/` when a dependency version changes upstream. Those stale jars sit on the classpath alongside the new ones (e.g. `zookeeper-3.9.4.jar` and `zookeeper-3.9.5.jar`) and cause `NoSuchMethodError` at runtime. Crucially, Helix/Pinot swallows the resulting `ExceptionInInitializerError` in ZK startup and surfaces a misleading `ZkTimeoutException: timeout: 1000` instead — which looks for all the world like a flaky port or timing issue. If you see that exception, **check `lib/` for duplicate versions of `zookeeper-*`, `helix-*`, `netty-*`, etc. first.** + - Existing appassembler output can retain old dependency jars after version changes. Inspect the build output and runtime classpath if versions changed or errors suggest a mismatch. If stale packaging is confirmed, clean only `pinot-perf` with `./mvnw -pl pinot-perf clean`, then run the package command below. Preserve prior diagnostic output before cleaning. - - **Use `-am` only on the first build.** After the first clean+package, upstream modules are populated; subsequent builds can skip `-am`. The worktree build in step 5 gets a fresh `target/` so doesn't have this problem. + - Keep `-am` unless dependency artifacts satisfy the evidence requirements in kb/skills/run-test.md. A prior reactor `package` alone does not install artifacts for a module-only invocation. Invocation is identical to step 6, just against the current tree's `lib/*`: ``` - ./mvnw -pl pinot-perf clean package -DskipTests -am - java -Xms4G -Xmx8G -cp 'pinot-perf/target/pinot-perf-pkg/lib/*' \ - org.openjdk.jmh.Main 'org.apache.pinot.perf.<BenchmarkClass>' <same JMH + jvmArgsAppend flags> \ - > .bench-compare/current-<sha-or-WIP>.txt 2>&1 + (cd "$bench_repo" && ./mvnw -pl pinot-perf -am package -DskipTests) \ + > "$bench_results_dir/current-build.txt" 2>&1 + (cd "$bench_repo" && java -Xms4G -Xmx8G -cp "$bench_repo/pinot-perf/target/pinot-perf-pkg/lib/*" \ + org.openjdk.jmh.Main 'org.apache.pinot.perf.<BenchmarkClass>' <same JMH + jvmArgsAppend flags>) \ + > "$bench_results_dir/current.txt" 2>&1 ``` -8. **Clean up the worktree.** `git worktree remove /tmp/pinot-bench-baseline --force`. Do this even if steps 6 or 7 failed. +8. **Clean up only the worktree created by this run.** After measurements finish or fail, check `bench_worktree_created`, inspect `git -C "$bench_repo" worktree list --porcelain` and `git -C "$bench_worktree" status --porcelain`, and verify the registered path and baseline SHA match this run. Once all its processes have stopped and there are no unexpected changes, remove it with: + ```sh + if [ "$bench_worktree_created" = true ]; then + git -C "$bench_repo" worktree remove "$bench_worktree" + fi + ``` + If ownership cannot be verified, unexpected changes exist, or removal refuses, retain the path and report the reason. Do not add `--force`, remove another run's directory, or delete the results directory. 9. **Diff the results.** Parse JMH's table output (the `Benchmark ... Score Error Units` lines) from both files. Produce a table: ``` @@ -74,13 +102,13 @@ Usage: ## Notes -- **Stale-jar trap is the #1 source of mysterious failures.** Pinot benchmarks that fail on a subsequent run of `/bench-compare` in the same repo almost always fail because of duplicate third-party jars in `pinot-perf/target/pinot-perf-pkg/lib/` — typically `zookeeper-X.jar` + `zookeeper-Y.jar` (or equivalent for helix, netty, guava) from different builds. The failure shows up as a deeply-wrapped `ZkTimeoutException: Unable to connect to zookeeper server within timeout: 1000` (or similar NoSuchMethodError swallowed into an infrastructure-looking error). First diagnostic when a second run fails: `ls pinot-perf/target/pinot-perf-pkg/lib/ | sort | awk -F- '{v=$NF; sub("\\.jar$","",v); k=$0; sub("-"v"\\.jar$","",k); print k}' | sort | uniq -d` to spot duplicates. The fix is always `./mvnw -pl pinot-perf clean package -DskipTests -am`, never `rm` individual jars. -- Worktrees require a clean `.git`. If the repo is in the middle of a rebase/merge, abort with a clear message. -- **JDK 21 needs the full `--add-opens` / `--add-exports` flag set** for any cluster-backed benchmark (extends `BaseClusterIntegrationTest`). Without them, ZK startup fails with `InaccessibleObjectException: ... module java.base does not "opens java.lang"`. Pass via `-jvmArgsAppend=...` to `org.openjdk.jmh.Main`; CI's `pinot_tests.yml` has the canonical list. +- Long builds and benchmark runs may execute asynchronously. Preserve logs and each process's exit code, provide meaningful progress, and await completion before comparing results. Keep baseline and current measurements sequential to avoid resource contention. +- Diagnose failures from the full exception chain, build logs, runtime classpath, and resource state. Duplicate dependency jars are one possible cause of linkage errors or wrapped ZK startup failures; a timeout alone does not establish that cause. Rebuild only when the evidence supports it, and retain unexplained failures as unresolved. +- An uncommitted working tree is supported. If an in-progress merge/rebase leaves the intended source state ambiguous, resolve the scope before building; do not abort or alter that Git operation automatically. +- Consult the current [integration-test workflow](../../.github/workflows/pinot_integration_tests.yml) for JVM module-access flags. Pass required benchmark-fork flags via `-jvmArgsAppend` to `org.openjdk.jmh.Main` using the same settings for baseline and current runs. - **The generated `pinot-<BenchmarkClass>.sh` scripts hard-code `-Xms24G -Xmx24G`.** Avoid them — use `java -cp 'lib/*' org.openjdk.jmh.Main <FQN>` directly with your own `-Xmx`. - **Not every benchmark has a generated script.** The appassembler programs list in `pinot-perf/pom.xml` covers ~21 of ~60 benchmark classes. The direct `java -cp` invocation works for any of them. -- JMH's `-l` (list benchmarks) flag **does not help here** — Pinot benchmark classes have custom `main()` entry points that construct `OptionsBuilder` directly, ignore CLI args, and plunge straight into `Runner.run(...)` which in turn kicks off `@Setup`. For `BenchmarkDictionary` this `@Setup` alone burns 5+ minutes building dictionaries. There is no fast sanity-check short of actually running the benchmark through `org.openjdk.jmh.Main` (which at least accepts `-wi 1 -i 1 -r 1s -w 1s` to minimise it). - Benchmarks must run on the same hardware, same JDK, same OS load. Warn the user if they're on battery power or running other heavy processes. -- Do not `sleep` between runs for "timing" reasons. If a second run fails, it is the stale-jar issue (above), not TIME_WAIT. I spent a long time chasing the timing hypothesis before spotting the classpath mismatch. +- Do not insert arbitrary sleeps or retries after a failed run; first inspect the failure and verify that its processes and exclusive resources have been released. - If the benchmark's output format isn't plain JMH (e.g. `BenchmarkVectorIndex` writes a custom report), don't try to parse it — just save both outputs and tell the user where they are, with a note that manual comparison is needed. - Never use `git stash` instead of a worktree. Stash can be lost if the second build fails and the user doesn't know to pop it. diff --git a/kb/skills/flaky-analyze.md b/kb/skills/flaky-analyze.md index ddc5213617fe..42a34117a7e2 100644 --- a/kb/skills/flaky-analyze.md +++ b/kb/skills/flaky-analyze.md @@ -17,15 +17,15 @@ If `gh` isn't available, report that and exit. Don't try to fall back to `curl` ## Procedure -1. **Parse the argument.** Extract class name (required), optional run count (default 20), optional PR filter. +1. **Parse the argument.** Extract class name (required), optional run count (default 20), optional PR filter. Use the count as a total budget across relevant workflows; cap at 50 unless the user authorized more. -2. **Find relevant workflow runs.** +2. **Find relevant workflow runs.** Select workflows from the target test's module and the current definitions under [`.github/workflows`](../../.github/workflows): `pinot_unit_tests.yml`, `pinot_integration_tests.yml`, or `pinot_quickstart_tests.yml` for the corresponding suites. Include compatibility workflows only when relevant (see Notes). Use `gh workflow list --repo apache/pinot` if the remote workflow names differ. Keep the total run budget across all selected workflows. ``` - gh run list --repo apache/pinot --workflow "Pinot Tests" --status failure --limit <N> --json databaseId,displayTitle,headBranch,headSha,createdAt,url + gh run list --repo apache/pinot --workflow <workflow-file> --status failure --limit <remaining-budget> --json databaseId,displayTitle,headBranch,headSha,createdAt,url ``` - If `--pr` is set, filter with `--branch` to the PR's head branch, or use `gh pr view <num> --json headRefName`. + If `--pr` is set, resolve it with `gh pr view <num> --repo apache/pinot --json headRefName,headRefOid`, filter with `--branch`, and verify the selected runs belong to that PR. -3. **For each failed run, find the failing jobs.** A "Pinot Tests" run has matrix jobs (test sets 1/2 × java 21 × unit/integration). Only some fail. +3. **For each failed run, find the failing jobs.** Unit and integration workflows have separate matrix jobs. Read the test set and JDK from each run rather than assuming the current matrix applies to older runs. ``` gh run view <run-id> --repo apache/pinot --json jobs ``` @@ -35,7 +35,7 @@ If `gh` isn't available, report that and exit. Don't try to fall back to `curl` ``` gh run view --job <job-id> --repo apache/pinot --log ``` - Each log line is prefixed by `<step-name>\tUNKNOWN STEP\t<timestamp>`. The log is large (tens of MB); pipe straight into ripgrep with these patterns — they are what Surefire/TestNG/GitHub Actions actually emit: + Log lines include step names and timestamps. Logs can be tens of MB; filter with ripgrep and retain enough surrounding context for the stack traces in step 5. Use these failure markers: - `\[ERROR\] Tests run: \d+, Failures: [1-9]` — the Surefire class-summary line when a test class had failures. The **fully qualified class name** is on the same line after `-- in `. - `\[ERROR\] Tests run: \d+, Failures: \d+, Errors: [1-9]` — same, with errors instead of failures. @@ -45,10 +45,10 @@ If `gh` isn't available, report that and exit. Don't try to fall back to `curl` **Do not grep for raw `ERROR` / `FAILED` / `Exception`** — Pinot's integration tests log these constantly at runtime (Helix rebalancer, consumer setup, etc.) and you'll drown in noise. The patterns above only match actual failure markers. - If none of those patterns match in a failing job's log, the test didn't fail at the test level — the job died for infrastructure reasons (timeout, OOM, runner cancel). Classify it as "infrastructure failure" and move on. + If none of those patterns match, inspect the job conclusion, annotations, and log availability. Classify a timeout, OOM, or runner failure only when evidence supports it; otherwise report an unknown cause or missing logs and move on within the budget. 5. **Extract structured failure records.** For each hit, record: - - Run id, PR number (if any), commit SHA, JDK version, test set (parseable from the job name like `Pinot Integration Test Set 1 (temurin-21)`). + - Workflow file/name, run id, PR number (if any), head SHA, JDK version, and test set or lane, as recorded by the job and its logs. Do not infer an older run's JDK or lane from today's workflow matrix. - The failing class FQN from the `-- in <FQN>` suffix of the summary line. - The failure message (typically the line containing `<<< FAILURE!` or the `AssertionError: ...` line that follows). - The top ~5 frames of the stack trace, taken from the ~30 lines following the `<<< FAILURE!` marker. @@ -59,13 +59,13 @@ If `gh` isn't available, report that and exit. Don't try to fall back to `curl` - Different stack traces → either multiple bugs or environmental flakiness. - Setup/timeout errors with no test code in the stack → likely infrastructure. -7. **Report.** Structure: +7. **Inspect relevant source and report.** Read the failing test and directly implicated source when needed to assess the hypothesis. Use the failing run's commit when source drift matters, and identify any mismatch with the current checkout. Structure: ``` ## Flaky analysis: <ClassName> Runs scanned: N (M with this test failing, K with unrelated failures) ### Failure cluster 1 — <exception type> at <top frame> (<count> occurrences) - Example (PR #<num>, JDK 21, test set 2): + Example (PR #<num>, JDK <version>, test set <set>): <short stack trace> Commits affected: <list of short SHAs> @@ -79,18 +79,18 @@ If `gh` isn't available, report that and exit. Don't try to fall back to `curl` - <specific, e.g. "reproduce locally with: /run-test ClassName", or "inspect X.java:123 which is top-of-stack"> ``` -8. **Do not propose a fix.** This skill is investigation, not remediation. End with "Want me to open the source file at the top-of-stack frame?" +8. **Finish with the evidence and remaining uncertainty.** This skill is report-only: do not change source or launch reproduction tests without remediation or reproduction being authorized. Do not add a routine confirmation question to read relevant source; that read is part of the investigation. ## Notes -- `gh run view --log` can be slow (10–60s per run) and returns large payloads. Cap total runs scanned at 50 unless the user asks for more. Run these fetches in parallel where possible; `gh` is rate-limited but stays under the limit for <50 runs. +- `gh run view --log` can be slow (10–60s per run) and returns large payloads. Cap total runs scanned at 50 unless the user asks for more. Fetch independent logs with bounded concurrency; handle rate limits within the investigation budget. - Don't write the raw logs to the repo. Stream them through grep and keep only the extracted records in memory. - `gh run view --log-failed` is not reliable here — it only returns the steps GitHub marked failed, which for Pinot's "Integration Test" step often just contains runner init lines before the actual Maven invocation. Always use `--log` + the patterns above. - To find failing *jobs* within a run without downloading its full log, use: ``` - gh api repos/apache/pinot/actions/runs/<run-id>/jobs --jq '.jobs[] | select(.conclusion=="failure") | {name, id: .databaseId}' + gh api --paginate repos/apache/pinot/actions/runs/<run-id>/jobs --jq '.jobs[] | select(.conclusion=="failure") | {name, id}' ``` Then pass the `id` as `--job <id>`. Avoids pulling all matrix logs. -- If the test doesn't appear in any failure log, report that directly: either the test isn't actually flaky on CI, or the search term is wrong. -- Results depend on log retention (GitHub keeps 90 days by default). Older flakes are invisible here — suggest checking the `surefire-reports-*` artifacts on master for long-term trends. -- The `Pinot Tests` workflow (file: `pinot_tests.yml`) is the right default. Also worth checking `Pinot Compatibility Regression Testing` and `Pinot Multi-Stage Query Engine Compatibility Regression Testing` workflows for integration tests that only run there — ask the user before querying those since they multiply the API calls. +- If the test does not appear in sampled failure logs, report the searched scope and lack of evidence. Check the selector and log availability; do not conclude the test is never flaky. +- State the sampled dates and any unavailable logs. Relevant Surefire artifacts can supplement missing console output if they are still retained; absence of retained evidence does not establish that the test passed. +- Query `pinot_compatibility_tests.yml` or `pinot_multi_stage_query_engine_compatibility_tests.yml` when relevant to the target test, within the same total run budget. Ask only before exceeding that budget. diff --git a/kb/skills/precommit.md b/kb/skills/precommit.md index 6693122d4cbd..9c21a7c0d823 100644 --- a/kb/skills/precommit.md +++ b/kb/skills/precommit.md @@ -2,49 +2,54 @@ Purpose: before pushing a commit or opening a PR, run all quality checks on the modules the current diff actually touches. Don't run them on the whole repo — that's slow and wasteful on a tree this size. -The five checks (in order): +The four required checks, where applicable to the selected files (in order): 1. `./mvnw spotless:apply -pl <modules>` — auto-formats code. 2. `./mvnw license:format -pl <modules>` — adds ASF headers to any new files. 3. `./mvnw checkstyle:check -pl <modules>` — validates style; fails hard. 4. `./mvnw license:check -pl <modules>` — validates headers; fails hard. -5. `./mvnw test-compile -pl <modules> -am -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true '-Dmaven.compiler.compilerArgs=-Xlint:all'` — compiles and checks for deprecation, unchecked casts, raw types, fallthrough, etc. Warnings are filtered to only lines added in the diff. -Steps 1 and 2 are auto-fixers. Steps 3 and 4 are validators — if they fail after the auto-fixers ran, report the failure with the exact offending file/line from the Maven output and stop. Do not try to manually patch style errors; fix the underlying issue or ask the user. Step 5 is a compiler check — if it produces warnings on newly added lines, report them. Prefer the non-deprecated replacement; suppress with `@SuppressWarnings` only with a comment explaining why the deprecated reference is required (e.g., backward-compat serialization, mixed-version SPI calls, testing the deprecated path). +For Java or build changes, also run `./mvnw test-compile -pl <modules> -am -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true` unless current equivalent compiler evidence exists. These supported flags show deprecation details and warnings enabled by the compiler configuration; they do not enable every lint category. Filter source warnings to lines added in the diff. + +Steps 1 and 2 are auto-fixers. Steps 3 and 4 are validators: fix clear violations within the authorized scope and rerun only checks invalidated by the fix. Ask only when a fix requires an unresolved semantic or compatibility decision or would alter unrelated work. Report remaining failures with the exact file/line. For compiler warnings on added lines, prefer the non-deprecated replacement; suppress with `@SuppressWarnings` only with a comment explaining why the deprecated reference is required (e.g., backward-compat serialization, mixed-version SPI calls, testing the deprecated path). + +Reuse passing check results only when their scope, files, dependencies, JDK, and relevant configuration remain valid. Record the command and result being reused. Each applicable required check must have valid passing results before pushing; do not repeat unaffected checks after a local fix. ## Procedure 1. **Find changed files.** - - If the user passed an argument (`staged`, `unstaged`, `branch`, or a path), use that as the scope. - - Default: union of staged + unstaged files vs. HEAD, plus any added-but-untracked `.java` / `.xml` / `.properties` files. - - Ignore: `target/`, `node_modules/`, generated sources, `**/*.md`, anything under `pinot-controller/src/main/resources/` (UI) unless the user explicitly asks — those aren't covered by the Maven plugins. + - Honor an explicit scope (`staged`, `unstaged`, `branch`, or a path). Otherwise choose the scope from the requested action. + - For a commit or a standalone precommit check, include this task's staged and unstaged changes plus intended untracked files. Preserve unrelated work. + - For a push, include the committed diff from the destination branch's current tip to `HEAD`; for a PR or `branch` check, use the merge-base with the target branch. Include authorized uncommitted changes that will be part of the delivery. A clean working tree does not mean there is nothing to check. + - Resolve the target from the user's request, the PR's base branch, or the configured push destination as appropriate; record the resolved ref and SHA. Ask only if the intended target remains ambiguous. Do not substitute `HEAD` for a missing branch baseline. + - Exclude generated output such as `target/` and `node_modules/`. Determine check applicability from the plugins' include/exclude rules in `pom.xml`; Markdown and UI files may still require license checks even though the Java formatter does not apply. 2. **Map files to modules.** For each changed file, walk up the directory tree until a `pom.xml` is found. The first directory containing a `pom.xml` that is *not* the repo root is the module. De-duplicate. - - If the only pom is the repo root, the user is touching top-level config — just run the checks at the root (no `-pl`). + - For root-only checks, use `-N` to avoid traversing the reactor. If both root and module files changed, check the root separately with `-N` and the modules with `-pl`. For compilation after a root build change, select affected modules from the dependency or configuration impact; do not expand to the whole reactor automatically. - Some plugin modules are nested two levels deep (e.g. `pinot-plugins/pinot-input-format/pinot-parquet`). Don't stop at an intermediate aggregator pom if it doesn't define the actual sources — walk up until you find the module that directly contains the changed file. -3. **Report the plan.** Print the list of detected modules in one line: `Modules: pinot-broker, pinot-common, pinot-plugins/pinot-input-format/pinot-parquet`. If there are no modules, say "No changed Java/XML files — nothing to do." and exit. +3. **Report the plan.** State the scope and baseline, then print the detected modules in one line: `Modules: pinot-broker, pinot-common, pinot-plugins/pinot-input-format/pinot-parquet`. If no checks apply to the selected files, report that scope and reason and exit. -4. **Run the auto-fixers.** Build a single `-pl` argument with comma-separated modules: +4. **Run the auto-fixers.** Use `<scope>` as `-pl <comma-separated-modules>` for module checks or `-N` for root checks: ``` - ./mvnw spotless:apply -pl <modules> - ./mvnw license:format -pl <modules> + ./mvnw spotless:apply <scope> + ./mvnw license:format <scope> ``` - Run each in the foreground. Track the number of files modified by each. If either fails with a non-build error (not a style error — those go through checkstyle), stop and surface the error. + Run the auto-fixers sequentially; they can modify the same files. Inspect the resulting diff and preserve unrelated user changes. Track the files modified by each. Diagnose execution failures before retrying; do not proceed to push with a failed check. 5. **Run the validators.** ``` - ./mvnw checkstyle:check -pl <modules> - ./mvnw license:check -pl <modules> + ./mvnw checkstyle:check <scope> + ./mvnw license:check <scope> ``` - If either fails, parse the Maven output, extract the file:line of each violation, and track them for the summary. Do not attempt to auto-fix checkstyle violations — they need human judgment. + If either fails, extract the file:line of each violation, fix clear violations within the authorized scope, and rerun the affected checks. Track any unresolved failures for the summary. -6. **Build the added-line set for compiler warning filtering.** This runs after the auto-fixers so that line numbers reflect the post-fix state (spotless may remove imports, shifting line numbers). Run `git diff --unified=0 HEAD -- <changed .java files>` and parse the `@@` hunk headers to extract the added line ranges. Build a map of `file → set of added line numbers`. For untracked `.java` files (new files not yet in git), `git diff` returns nothing — treat all lines as added (use `wc -l` to get the line count and add 1 through N to the set). +6. **Decide whether compiler validation is needed.** Skip steps 6–7 for changes unrelated to Java or the build, or when equivalent compiler evidence remains valid. Otherwise, build the added-line set after the auto-fixers so that line numbers reflect the post-fix state. Use the same diff scope as step 1 with `--unified=0`, and parse the `@@` hunk headers into `file → set of added line numbers`. For untracked `.java` files, treat all lines as added. 7. **Run the compiler check.** ``` - ./mvnw test-compile -pl <modules> -am -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true '-Dmaven.compiler.compilerArgs=-Xlint:all' + ./mvnw test-compile -pl <modules> -am -Dmaven.compiler.showDeprecation=true -Dmaven.compiler.showWarnings=true ``` - This is the only step that uses `-am` — compilation needs upstream dependencies built, unlike the other steps. Do not use `clean` — incremental compilation still emits warnings for all files in the module, and the per-line filter (step 6) handles pre-existing warnings. Using `clean` would break modules with generated sources (e.g., JavaCC in `pinot-common`) and adds significant overhead on deep modules. If warnings seem missing (e.g., stale `target/` from a different branch), the user can manually run `./mvnw clean generate-sources test-compile -pl <modules> -am ...` to force full recompilation. + This is the only step that uses `-am` — compilation needs current upstream dependencies, unlike the other checks. See kb/skills/run-test.md for the dependency evidence required to omit `-am`. Avoid `clean` by default. Reuse current compilation evidence from a test or build when its scope and configuration match; report the warning flags it used. An incremental "nothing to compile" result provides no fresh warning output. Do not force recompilation solely to collect more warnings; state any coverage limit. Rebuild the affected scope, including required source generation, when changed inputs or evidence of stale outputs invalidate prior compilation. Parse the output for `[WARNING]` lines. **Filter to only added lines from the diff** — for each warning of the form `[WARNING] /path/File.java:[line,col] <message>`, check whether that file and line number appear in the added-line set from step 6. Only report warnings that match. This avoids surfacing pre-existing warnings when a contributor edits a file that already has them. @@ -52,7 +57,7 @@ Steps 1 and 2 are auto-fixers. Steps 3 and 4 are validators — if they fail aft For deprecation warnings: prefer the non-deprecated replacement API. If removing the deprecated reference is not feasible (e.g., backward-compat serialization, mixed-version SPI calls, testing the deprecated path), suppress with `@SuppressWarnings("deprecation")` and a comment explaining why. -8. **Print summary report.** Always print the full report, even when all checks pass: +8. **Print summary report.** Report the required checks, any reused evidence, and whether the compiler check ran or was skipped. Include unresolved issues and files changed by auto-fixers. A compact report is enough when all checks pass; for failures, use: ``` ## Pre-commit Summary — <n> modules @@ -63,12 +68,12 @@ Steps 1 and 2 are auto-fixers. Steps 3 and 4 are validators — if they fail aft | license:format | OK | 0 files needed headers | | checkstyle:check | PASS | | | license:check | PASS | | - | test-compile -Xlint | FAIL | 2 warnings on new lines | + | test-compile | FAIL | 2 warnings on new lines | - ### Auto-fixed (review before staging) + ### Auto-fixed - spotless reformatted: File1.java, File2.java - ### Not fixed (requires manual action) + ### Unresolved - `SomeClass.java:45` — [deprecation] Foo.bar() is deprecated, use Foo.baz() - `OtherClass.java:12` — [unchecked] unchecked cast to List<String> ``` @@ -78,14 +83,15 @@ Steps 1 and 2 are auto-fixers. Steps 3 and 4 are validators — if they fail aft - **OK** — auto-fixer ran but nothing needed fixing - **PASS** — validator passed with no violations - **FAIL** — validator or compiler found issues + - **REUSED** — a passing result remains valid for the current scope + - **SKIP** — the check does not apply to the selected files; state why The report must include: - - The full table for all 5 checks, every time - Every unfixed issue with file:line and what to do about it - - Every auto-fixed file so the user can review before staging + - Every auto-fixed file - For deprecation: the deprecated API and its replacement (if known) - Do not stage auto-fixed files; that's the user's choice. + If the user authorized a commit, review and stage only this task's changes, including its formatting fixes. Preserve unrelated staged and unstaged changes. Otherwise leave changes unstaged. ## What each step actually enforces @@ -95,15 +101,14 @@ Knowing this matters for diagnosing failures: - **`license:check/format`**: the ASF header from `HEADER` (repo root), applied to `.java`, `.xml`, `.js`, `.sh`, `.md`, etc. Many file types are excluded — see the `licenseSets/excludes` block in the parent `pom.xml`. - **`checkstyle:check`**: rules from `config/checkstyle.xml`. The common ones contributors trip: `LineLength` (120 chars), `AvoidStarImport`, `AvoidStaticImport`, `HideUtilityClassConstructor`, `NeedBraces`. Output format is `[WARNING] <file>:[<line>] (<group>) <RuleName>: <message>` — parse that when surfacing violations. - **`license:check`** runs after `license:format` to confirm every touched file now has the header, including files the user only renamed (the plugin keys off content, not git status). -- **`test-compile -Xlint:all`**: uses `test-compile` (not just `compile`) so both `src/main/` and `src/test/` sources are compiled and all warnings are emitted. Do not use `clean` — it wipes generated sources (e.g., JavaCC in `pinot-common`) and adds significant overhead on deep modules. Incremental compilation still emits warnings for the entire module; the per-line filter handles pre-existing warnings. The Java compiler flags any reference to `@Deprecated` classes/methods/fields from any dependency (Pinot internal or third-party jars), plus unchecked casts, raw types, fallthrough in switch, and other warning categories. Output format: `[WARNING] /path/File.java:[line,col] <message>`. Warnings are filtered to added lines in the diff only — not just by file, but by the specific line numbers from `git diff --unified=0`. +- **`test-compile` with warning flags**: covers `src/main/` and `src/test/` compilation when applicable and reports the warnings enabled by the compiler configuration, including deprecation details. It does not promise all lint categories. Output format: `[WARNING] /path/File.java:[line,col] <message>`. Filter source warnings to added lines using step 6 and report compilation/evidence reuse as described in step 7. ## Notes - Always use `./mvnw`, never a system `mvn`. The repo's CLAUDE.md is explicit on this. - Don't pass `-am` for steps 1–5 — that builds upstream dependencies too, which defeats the purpose of scoping. Only step 7 (compile) needs `-am` because javac needs dependency jars on the classpath. -- Run sequentially, not in parallel. Spotless and license:format may both modify the same files; ordering matters. -- When `spotless:apply` removes an unused import, it leaves a *leftover blank line* where the import used to be. This is harmless (checkstyle does not flag it), but if the user cares about the cosmetic double-blank, they'll need to hand-clean after the skill runs. Mention this in the report if spotless touched any files. +- Run auto-fixers sequentially. Read-only validators may run independently after all edits settle. - If the user says `/precommit all`, run on the whole repo (no `-pl`). Warn that this is slow (several minutes). -- Long builds: steps 1–5 are fast (<30s warm). Step 7 (test-compile with `-am`) is slower (~7–90s depending on module depth and Maven cache state). Deep modules with many upstream deps may take longer on a cold cache. Use `run_in_background` only if the user explicitly asks — otherwise show progress inline. +- Long commands may run asynchronously without additional permission. Save logs and the Maven exit code, report meaningful progress, and use the time for independent work. Await completion before claiming a check passed or pushing. - The `license:check` and `checkstyle:check` goals return Maven exit code `1` on violations. If you're capturing the output with shell chaining like `... | tail`, the *tail* pipeline's exit code will mask Maven's — always record Maven's exit code separately, e.g. with `set -o pipefail` or by capturing `${PIPESTATUS[0]}`. - The compiler warning filter (step 7) uses the added-line set from step 6, not just the changed-file list. This is critical — per-file filtering would surface pre-existing warnings in files the contributor merely edited, which is unfair. Per-line filtering ensures only warnings on newly added code are reported. diff --git a/kb/skills/quickstart.md b/kb/skills/quickstart.md index 729a2ce2c212..5f64e45d0897 100644 --- a/kb/skills/quickstart.md +++ b/kb/skills/quickstart.md @@ -7,47 +7,37 @@ Usage: - `/quickstart batch` — batch mode (offline table with sample data). - `/quickstart hybrid` — hybrid table (offline + realtime). - `/quickstart streaming` — realtime consumption from an embedded Kafka. -- `/quickstart upsert-streaming` — upsert table on Kafka. -- `/quickstart partial-upsert-streaming` — partial-upsert table on Kafka. -- `/quickstart json-index-batch` / `json-index-streaming` — JSON index demos. -- `/quickstart complex-type-handling-offline` / `complex-type-handling-streaming` — complex type demos. - `/quickstart auth` / `auth-zk` — auth-enabled variants. +Batch includes the offline JSON-index and complex-type examples. Streaming includes JSON-index, complex-type, full-upsert, and partial-upsert examples. These no longer have separate generated scripts in this checkout. + ## Procedure -1. **Find the quickstart script.** Look in order: +1. **Resolve the mode before building.** Check the generated program names in [`pinot-tools/pom.xml`](../../pinot-tools/pom.xml). For a legacy or feature-specific request, consult the quickstart implementation's `types()` and examples under [`org/apache/pinot/tools`](../../pinot-tools/src/main/java/org/apache/pinot/tools) and map it to the supported batch or streaming script. Deprecated type aliases are not separate script names. For an unsupported mode, report the available modes without building. + +2. **Find the selected script and its matching artifacts.** Look in order, reusing a build only when it matches the requested checkout and scope: - `build/bin/quick-start-<mode>.sh` (produced by `-Pbin-dist`) - `pinot-tools/target/pinot-tools-pkg/bin/quick-start-<mode>.sh` (produced by a plain `./mvnw package` of `pinot-tools`) - If neither exists, the binary distribution hasn't been built. - -2. **If the script is missing, offer to build.** Ask the user: - > Quickstart scripts not found. Build them now? - > - Full bin-dist (recommended for first time): `./mvnw clean install -DskipTests -Pbin-dist -Pbuild-shaded-jar` (~10 min) - > - Just pinot-tools: `./mvnw -pl pinot-tools -am package -DskipTests` (~3 min) +3. **Build missing or outdated artifacts within the requested scope.** A request to launch quickstart authorizes its routine local prerequisites. Run `./mvnw -pl pinot-tools -am package -DskipTests` asynchronously, retaining logs and the Maven exit code. After success, use the script from `pinot-tools/target/pinot-tools-pkg/bin/`. Use the full binary distribution only when the requested deliverable requires it. If the expected script is still missing, inspect the packaging output instead of repeating the build. - Run the one they pick in the foreground. If they're re-running after a prior build, `build/bin/` should already exist. - -3. **Validate the mode.** If the user passed an unrecognized mode, list the available scripts: +4. **Run the selected script in the background.** Quickstart processes run indefinitely. Use the execution tool's background/session support: ``` - ls build/bin/quick-start-*.sh 2>/dev/null || ls pinot-tools/target/pinot-tools-pkg/bin/quick-start-*.sh + "<resolved-script-path>" ``` + Record the session handle, process IDs, command, start time, and log path. Track any Java child process so later cleanup targets only this launch. Check for occupied ports before launching; reuse an existing cluster only when it matches the requested mode and scope. Do not kill an unrelated process to free a port. -4. **Run the script in the background.** Quickstart processes run indefinitely; they're servers. Use `run_in_background` so the user can keep working: - ``` - build/bin/quick-start-<mode>.sh - ``` - Capture the shell id so the user can check output later. +5. **Verify readiness within a deadline.** Use an existing startup budget, or allow up to 120 seconds after launch. Check process state, startup logs, and `GET http://localhost:9000/tables` with a short request timeout. Once the expected table is available, run one small query from the selected mode's example and check for query exceptions and an expected result. Use the mode's credentials when authentication is enabled. A fixed delay or controller log line alone does not prove the cluster can serve queries. Stop checking on success, process failure, or the deadline; report any remaining initialization without an open-ended wait. -5. **Report how to use it.** Once started (wait ~30s or until logs show "Pinot Controller started"), tell the user: +6. **Report how to use it.** Include readiness and query results, then: - Controller UI: http://localhost:9000 - Query console: http://localhost:9000/#/query - - To stop: kill the background shell (provide the id). - - Logs: printed to the background shell's stdout. + - To stop: use the recorded session/process IDs after verifying they still belong to this launch. + - Logs: provide the retained log path or session handle. ## Notes -- Quickstart processes are long-running. Do not poll with `sleep` loops. The user can check status via the HTTP UI. -- Multiple quickstarts can't run simultaneously — they all bind to the same ports (9000, 8000, 7050, 8098, 8099). If a run fails with "Address already in use", check for an existing quickstart and ask the user before killing it. +- Multiple default quickstarts use the same ports (9000, 8000, 7050, 8098, 8099). If a port is occupied and the existing cluster cannot satisfy the request, report the conflict. Ask only if stopping that other process is necessary and not already authorized. Never use a broad process-name kill command. - The auth quickstart uses a default admin/verysecret credential; mention this if the user picks `auth` or `auth-zk`. -- On macOS with JDK 21+, quickstarts may need extra `--add-opens` flags; the shipped scripts handle this already, so don't add flags unless the user reports a module-access error. +- The generated quickstart launchers request an initial 4 GB heap; account for that when checking local resources. +- Use the JDK required by `AGENTS.md` and the shipped launcher's JVM flags. Add module-access flags only when an observed launcher or runtime error shows they are needed. diff --git a/kb/skills/review-architecture.md b/kb/skills/review-architecture.md index 58b7d6031519..516cb8a64106 100644 --- a/kb/skills/review-architecture.md +++ b/kb/skills/review-architecture.md @@ -1,11 +1,10 @@ # review-architecture -You are a specialized reviewer for **Apache Pinot domain 3: Code Architecture & Module Design**. Read `kb/code-review-principles.md` section 3 and `CLAUDE.md`. +Review **Apache Pinot domain 3: Code Architecture & Module Design**. Read the applicable parts of section 3 in +`kb/code-review-principles.md` and relevant repository conventions not already loaded. Reuse material already read. -Severity: -- **CRITICAL** — circular dependency across modules; public SPI added without considering backward compat; broker code reaching into server-only internals. -- **MAJOR** — missing abstract base where >1 implementation exists; duplicated utility logic instead of extending a shared one; plugin pulls non-plugin deps into its module. -- **MINOR** — package misnamed for its role; helper utility could live one module higher. +Use the canonical severity definitions and Review Delivery rules in `kb/code-review-principles.md`. Assess demonstrated +impact; pattern matches are investigation triggers, not findings or automatic severity assignments. ## 1. Broad scan @@ -13,16 +12,18 @@ Severity: - New interfaces or abstract classes. - New POM `<dependency>` entries; check the module and the scope. - Imports that cross module roots (`pinot-broker` importing `org.apache.pinot.core.query.executor.ServerQueryExecutor`, or `pinot-common` importing `pinot-core`). -- Two or more implementations of the same SPI: check for a missing abstract base. +- Multiple implementations of the same SPI: check whether shared behavior warrants reuse without coupling distinct semantics. - Any file moved into or out of `pinot-spi/` (binary contract surface). ## 2. Deep analysis - **C3.x** Confirm module layering: `pinot-spi` → `pinot-common` → `pinot-segment-spi` → `pinot-segment-local` → `pinot-core` → `pinot-query-runtime` / `pinot-broker` / `pinot-server` / `pinot-controller`. Edges must flow one-way. -- Prefer abstract base over interface-with-default when >1 impl exists and logic is shared (see the AbstractResponseStore pattern). +- Consider an abstract base or shared helper when implementations share behavior and lifecycle contracts. Keep separate + implementations when reuse would couple different semantics or introduce unwanted dependencies. - Plugin modules under `pinot-plugins/` must not be depended-on by core code. Verify the direction. - New REST resources belong in `pinot-controller` or `pinot-broker`, never cross-wired. -- Utility classes: if a helper mirrors existing functionality (e.g., a second `PartitionIdUtils`), flag duplication and recommend consolidation. +- Utility classes: compare semantics and dependencies before recommending consolidation of similar helpers. Report a + concrete maintenance or correctness risk, not resemblance alone. - Shaded-jar impacts: flag if a new transitive dep clashes with an existing shaded package. ## 3. Findings diff --git a/kb/skills/review-concurrency-state.md b/kb/skills/review-concurrency-state.md index 8597d7143566..4ca069933f75 100644 --- a/kb/skills/review-concurrency-state.md +++ b/kb/skills/review-concurrency-state.md @@ -1,17 +1,17 @@ # review-concurrency-state -You are a specialized reviewer for **Apache Pinot domain 2: State Management & Concurrency**. Read `kb/code-review-principles.md` section 2 and `CLAUDE.md` before analyzing. +Review **Apache Pinot domain 2: State Management & Concurrency**. Read the applicable parts of section 2 in +`kb/code-review-principles.md` and relevant repository conventions not already loaded. Reuse material already read. -Severity: -- **CRITICAL** — data race, atomicity violation (wipe-before-install), IdealState write without version check, visibility bug on shared mutable state. -- **MAJOR** — unnecessary lock widening, striped lock without measured contention, check-then-act race even if rare. -- **MINOR** — over-synchronization, missing `volatile` where `final` would be safer, comment omission on thread-safety contract. +Use the canonical severity definitions and Review Delivery rules in `kb/code-review-principles.md`. Assess demonstrated +impact; pattern matches are investigation triggers, not findings or automatic severity assignments. ## 1. Broad scan - Added/removed `synchronized`, `volatile`, `AtomicReference`, `AtomicLong`, `ReentrantLock`, `StampedLock`, `ConcurrentHashMap`, `CopyOnWriteArrayList`. - `get` followed by `put` / `remove` on concurrent maps (check-then-act pattern). -- Helix `IdealState` / `ExternalView` reads without version checks, or `setIdealState` without `dataAccessor.getProperty(...).getStat()`. +- Helix read-modify-write paths: check that writes validate the version read, directly or through the update helper. + Ordinary `IdealState` / `ExternalView` reads do not require version checks. - `@GuardedBy` annotations added or removed. - Registration of observers / listeners (callbacks, MetricsRegistry, segment lifecycle listeners) without clear lifetime documentation. - Background threads: `Executors.new*`, `ScheduledExecutorService`, `Thread`. Check shutdown path (`awaitTermination` then `shutdownNow`). @@ -22,10 +22,14 @@ Severity: For each hit, apply the KB's concurrency principles: - **C2.1 — Atomic transitions.** Never wipe old metadata before the new state is durably installed. Pattern: prepare-new → swap-reference → cleanup-old. Flag eager deletes. -- **C2.2 — Thread-safety conservatism.** Default to explicit synchronization. If replacing `synchronized` with `AtomicReference` or `CHM.compute`, verify the visibility story holds across all callers. -- **C2.3 — Race analysis for lock changes.** When a lock's scope is narrowed or removed, walk through interleavings with other threads that touch the same state. Flag if the walk-through isn't in the PR description. -- **C2.4 — Version-checked writes.** Shared state in ZK (IdealState, IdealStateConfig, TableConfig, Schema) must be written with optimistic locking (ZK node version); reject blind writes. -- **C2.5 — Check-then-act on atomics is still racy.** `if (!map.containsKey(k)) map.put(k, v)` is a bug — must be `putIfAbsent` / `computeIfAbsent`. +- **C2.2 — Thread-safety conservatism.** Establish ownership, thread confinement, publication, and lifecycle contracts + before proposing synchronization. For lock or atomic-operation changes, verify visibility and atomicity across callers. +- **C2.3 — Race analysis for lock changes.** Walk through the key interleavings with other threads touching the same state. + Report reachable unsafe interleavings as findings and unresolved contracts as coverage limits; missing PR prose alone is not a race finding. +- **C2.4 — Version-checked writes.** Shared ZK read-modify-write operations (IdealState, IdealStateConfig, TableConfig, + Schema) must preserve concurrent updates through version checks or an equivalent update helper; flag unprotected writes. +- **C2.5 — Check-then-act on atomics is still racy.** For concurrent access, verify whether + `if (!map.containsKey(k)) map.put(k, v)` requires `putIfAbsent` / `computeIfAbsent` or is protected by a wider invariant. - **C2.6 — Shared observers.** When an observer is registered from multiple paths or called concurrently, the handler must be idempotent and its mutable state must be published safely. Also check lifecycle: every `new ExecutorService` needs a clear shutdown path in `close()` / stop hook. diff --git a/kb/skills/review-config-backcompat.md b/kb/skills/review-config-backcompat.md index 1730eb9fe574..413597c41f51 100644 --- a/kb/skills/review-config-backcompat.md +++ b/kb/skills/review-config-backcompat.md @@ -1,11 +1,10 @@ # review-config-backcompat -You are a specialized reviewer for **Apache Pinot domain 1: Configuration & Backward Compatibility**. Read `kb/code-review-principles.md` (section "1. Configuration & Backward Compatibility") and `CLAUDE.md` before analyzing. +Review **Apache Pinot domain 1: Configuration & Backward Compatibility**. Read the applicable parts of section 1 in +`kb/code-review-principles.md` and relevant repository conventions not already loaded. Reuse material already read. -Severity (from the KB): -- **CRITICAL** — must fix: removed/renamed config key with no legacy fallback; widened SPI signature; renamed enum/DataType/schema type; Protobuf field-number reuse; DataTable/segment version bump without dual-read. -- **MAJOR** — should fix: new feature ships ON by default; multi-level override not validated; missing `@Deprecated` on legacy alias. -- **MINOR** — quality: config namespace inconsistent; constant name mismatches string value; comment misses the rollout plan. +Use the canonical severity definitions and Review Delivery rules in `kb/code-review-principles.md`. Assess demonstrated +impact; pattern matches are investigation triggers, not findings or automatic severity assignments. ## 1. Broad scan @@ -31,7 +30,8 @@ For each hit, apply the trigger match from the KB and compare the change against - **C1.3** For SPI signature changes: could an existing plugin compiled against an older version still link? Prefer overloads over widening. - **C1.4** For reverts: does the commit reference the original PR and explain the failure mode? - **C1.5** For new `isXxxEnabled()`-style validation: does it resolve through table → instance → default override chain? Use the `Enablement` enum where it exists. -- **C1.6** New feature flag defaults to OFF (`false` for `enableXxx`, or `false` for `disableXxx` = enabled). +- **C1.6** New behavior defaults OFF: `enableXxx=false`, or `disableXxx=true` for an established disable-style key. + Preserve existing defaults unless intentionally changed and validated; check effective behavior, not the literal boolean. - **C1.7** Config namespace follows existing patterns (`pinot.broker.*`, `pinot.query.sse.*`, dot-separated lowercase). - Wire format: DataTable / segment-version bumps must keep the reader able to decode prior versions; confirm dual-read is tested. - Rolling upgrade: is there a written rolling-upgrade note for backward-incompat label PRs (broker-first vs controller-first)? diff --git a/kb/skills/review-correctness-nulls.md b/kb/skills/review-correctness-nulls.md index cc3ece01cd40..ac9410ad3c0f 100644 --- a/kb/skills/review-correctness-nulls.md +++ b/kb/skills/review-correctness-nulls.md @@ -1,11 +1,10 @@ # review-correctness-nulls -You are a specialized reviewer for **Apache Pinot domain 5: Correctness & Safety**. Read `kb/code-review-principles.md` section 5 and `CLAUDE.md`. +Review **Apache Pinot domain 5: Correctness & Safety**. Read the applicable parts of section 5 in +`kb/code-review-principles.md` and relevant repository conventions not already loaded. Reuse material already read. -Severity: -- **CRITICAL** — silent wrong results (precision loss, missing enum case handled as default), memory leak on segment destroy, null dereference that crashes a query, missing null-bitmap update. -- **MAJOR** — use of double fallback where polymorphic primitive dispatch exists; exception swallowed without logging; unchecked `Optional.get()`. -- **MINOR** — `@Nullable` missing on a return that may be null; redundant null check. +Use the canonical severity definitions and Review Delivery rules in `kb/code-review-principles.md`. Assess demonstrated +impact; pattern matches are investigation triggers, not findings or automatic severity assignments. ## 1. Broad scan @@ -19,8 +18,10 @@ Severity: ## 2. Deep analysis -- **C5.x** Null handling: dispatch on `getStoredType()`, update null-value-vector bitmap on insert/delete, test both `null-handling-enabled=true` and `false` paths. -- **Exhaustive switches**: prefer `EnumSet.allOf` confirmation or a `default: throw new IllegalArgumentException("Unsupported " + type)`; silent fallthrough is a CRITICAL risk (see PR 18176 `IVF_ON_DISK` case). +- **C5.x** Null handling: dispatch on `getStoredType()`, update null-value-vector bitmap on insert/delete, and check coverage + of both `null-handling-enabled=true` and `false` when the change affects both paths. Reuse existing coverage. +- **Exhaustive switches**: prefer `EnumSet.allOf` confirmation or a `default: throw new IllegalArgumentException("Unsupported " + type)`; + trace the effect of silent fallthrough before assigning severity (see PR 18176 `IVF_ON_DISK` case). - **Precision**: INT/LONG windows must not coerce to double; BIG_DECIMAL requires its own aggregator. Flag coercions that may lose precision past 2^53. - **Resource cleanup**: `close()` must actively trim state, not rely on GC. Even if dangling refs are rare, explicit cleanup is the norm (see PR 18204 bitmap leak). - **Error messages**: `Preconditions.checkState`, `IllegalArgumentException`, `IllegalStateException` must include the offending value — not opaque. @@ -28,9 +29,10 @@ Severity: ## 3. Findings -Tag `skill: review-correctness-nulls`, cite `C5.x`, use `[BUG-CORR]` for unnamed bugs. For precision / silent-wrong-result risks, always classify CRITICAL. +Tag `skill: review-correctness-nulls`, cite `C5.x`, use `[BUG-CORR]` for unnamed bugs. Demonstrate the input and path that +can produce precision loss or silent wrong results, and distinguish confirmed impact from an unverified risk. ## When to defer to the developer -- Null path is explicitly out-of-scope in the PR description and a follow-up issue is linked. +- Null behavior is unaffected and existing coverage preserves its contract. A scope statement does not excuse a newly introduced null-handling defect. - `default` branch falls through to a documented best-effort path (rare; must be justified). diff --git a/kb/skills/review-naming-api.md b/kb/skills/review-naming-api.md index 79c7f5edbe13..e657acfbc929 100644 --- a/kb/skills/review-naming-api.md +++ b/kb/skills/review-naming-api.md @@ -1,11 +1,10 @@ # review-naming-api -You are a specialized reviewer for **Apache Pinot domain 7: Naming & API Design**. Read `kb/code-review-principles.md` section 7 and `CLAUDE.md`. +Review **Apache Pinot domain 7: Naming & API Design**. Read the applicable parts of section 7 in +`kb/code-review-principles.md` and relevant repository conventions not already loaded. Reuse material already read. -Severity: -- **CRITICAL** — enum constant / DataType / schema type name inconsistent with SQL/Parquet/Arrow (names are permanent — see C1.2); public API with same-module name collision. -- **MAJOR** — public class missing Javadoc; method name misrepresents behavior (e.g., `get` that mutates); inconsistent REST naming vs. existing resources. -- **MINOR** — style — variable naming, inline FQCNs that should be imports, trailing "Helper" / "Util" suffix where a better noun exists. +Use the canonical severity definitions and Review Delivery rules in `kb/code-review-principles.md`. Assess demonstrated +impact; pattern matches are investigation triggers, not findings or automatic severity assignments. ## 1. Broad scan @@ -22,7 +21,7 @@ Severity: ## 2. Deep analysis - **C7.x** Confirm name matches behavior. `get*` should not mutate. `isXxx` / `hasXxx` for booleans. `toXxx` / `fromXxx` for conversions. -- Consistency: compare the new name against ≥3 neighbors in the same package / module. +- Consistency: consult the most relevant neighbors in the same package / module when the naming convention is unclear. - Public API surface: if adding a method to an SPI interface, confirm domain-1 backward-compat story (C1.3). - Javadoc: new public classes must describe behavior and thread-safety. - Imports: `com.foo.Bar foo = new com.foo.Bar()` → use import. @@ -30,7 +29,7 @@ Severity: Checkstyle-blocked `Collections.emptyList`, `Collections.emptySet`, or `Collections.emptyMap` usage. Do not recommend blanket bans for `Collections.singleton*`, but only allow them for explicit null element/key/value arguments; check whether empty collections flow to mutating callers before replacement. -- CLAUDE.md checks: license header, Java 21 target (Java 11 bytecode for SPI/client modules), SLF4J logger pattern. +- Repository checks: license header, build JDK and bytecode targets from `AGENTS.md`, SLF4J logger pattern. ## 3. Findings diff --git a/kb/skills/review-performance.md b/kb/skills/review-performance.md index a530f375603b..f7a4f79fc4f6 100644 --- a/kb/skills/review-performance.md +++ b/kb/skills/review-performance.md @@ -1,28 +1,30 @@ # review-performance -You are a specialized reviewer for **Apache Pinot domain 4: Performance & Efficiency**. Read `kb/code-review-principles.md` section 4 and `CLAUDE.md`. +Review **Apache Pinot domain 4: Performance & Efficiency**. Read the applicable parts of section 4 in +`kb/code-review-principles.md` and relevant repository conventions not already loaded. Reuse material already read. -Severity: -- **CRITICAL** — documented benchmark regresses significantly; per-row allocation in top-level operator loop; large synchronized block on query-path singleton. -- **MAJOR** — missing primitive fast-path (e.g., reusing `Long` boxing); unnecessary intermediate collection on the scan path; string concat inside a per-row loop. -- **MINOR** — minor inefficiency off the hot path; logging at INFO inside a tight loop. +Use the canonical severity definitions and Review Delivery rules in `kb/code-review-principles.md`. Assess demonstrated +impact; pattern matches are investigation triggers, not findings or automatic severity assignments. ## 1. Broad scan - Per-row methods: search for `getInt`, `getLong`, `getDouble`, `getString`, `getBytes`, `getValue`, `transform`, `filter`, `accept` inside operator / transform / aggregator files. - Allocations in loops: `new `, `Arrays.asList`, `Collections.singletonList`, `String.format`, `"x" + y`, lambdas capturing variables. - Boxing: use of `Integer`, `Long`, `Double`, `Boolean` where `int`, `long`, `double`, `boolean` would do; `Map<K, Integer>`-style in hot code. -- Virtual dispatch in hot loops: fields typed as an interface where a concrete class would let JIT inline. +- Dispatch in hot loops: investigate polymorphic call sites when profiling or code-path evidence suggests an inlining issue; + an interface-typed field alone does not demonstrate a regression. - `synchronized` / lock acquisition inside for-loops on the scan path. - Missing type-specific aggregator (e.g., `Sum` falls back to `BigDecimal` when `Long` would suffice — see recent PRs on `SumLongWindowValueAggregator`). - `ByteBuffer.duplicate()` / `slice()` in loops. ## 2. Deep analysis -- **C4.x** Ask: does this code run per-row, per-segment, or per-query? Apply the budget of each (per-row: zero allocations, no boxing, no virtual dispatch where avoidable). +- **C4.x** Establish whether code runs per-row, per-segment, or per-query. Assess added allocation, boxing, dispatch, + and contention against the relevant workload; do not infer runtime cost from syntax alone. - Check for type-dispatch on `getStoredType()` rather than a double-coercion fallback. Precision loss past 2^53 is a correctness issue but also a perf giveaway (extra unbox + cast). - If the PR claims a perf gain, confirm a benchmark is attached or point to the `/bench-compare` skill as a next step. -- If a benchmark shows a regression > 5%, flag CRITICAL regardless of other merits. +- Evaluate benchmark regressions using comparable workloads, environments, repeated measurements, noise, and affected + production paths. Assign severity from demonstrated impact, not a fixed percentage alone; report missing evidence. - Avoid introducing `LOGGER.debug(String.format(...))` in per-row loops — even when debug is off, formatting may be eager. ## 3. Findings diff --git a/kb/skills/review-process-scope.md b/kb/skills/review-process-scope.md index 7ab55add22f9..9e1fa45fa189 100644 --- a/kb/skills/review-process-scope.md +++ b/kb/skills/review-process-scope.md @@ -1,19 +1,21 @@ # review-process-scope -You are a specialized reviewer for **Apache Pinot domain 8: Process & Scope**. Read `kb/code-review-principles.md` section 8 and `CLAUDE.md`. +Review **Apache Pinot domain 8: Process & Scope**. Read the applicable parts of section 8 in +`kb/code-review-principles.md` and relevant repository conventions not already loaded. Reuse material already read. -Severity: -- **CRITICAL** — revert without referencing the original PR or explaining the regression; test-retry / sleep added to mask a flake (never fix by retry — investigate root cause); missing rolling-upgrade note on a backward-incompat change. -- **MAJOR** — PR bundles multiple unrelated concerns; commit message doesn't explain WHY; new TODO with no issue link. -- **MINOR** — PR title style; label missing. +Use the canonical severity definitions and Review Delivery rules in `kb/code-review-principles.md`. Assess demonstrated +impact; pattern matches are investigation triggers, not findings or automatic severity assignments. ## 1. Broad scan -- Diff size + module count. If > 500 lines or > 4 modules, flag for scope review. -- Commit messages (`git log <base>..HEAD`): check each for a WHY clause. +- Use diff size and module count to plan coverage. Report scope issues only when the diff includes unrelated concerns, + not because it crosses a fixed size threshold. +- Commit messages (`git log <base>..HEAD`): check that the purpose of the change is understandable from the available context. - New `// TODO` / `// FIXME` — confirm each has a linked issue. - Test retry patterns: `@Test(retryAnalyzer = ...)`, `Thread.sleep` added in tests, `@Flaky` annotations. - PR title / labels if available. +- Required checks for the reviewed SHA: apply C6.1, classify failures, and report pending or failing merge gates. + Read-only review does not change Git state or retry CI; authorized remediation addresses attributable failures within scope. ## 2. Deep analysis @@ -25,7 +27,8 @@ Severity: ## 3. Findings -Tag `skill: review-process-scope`, cite `C8.x`, use `[PROC]` for process nits. Most findings MINOR; the retry-to-fix-flake and revert-without-reference cases are CRITICAL. +Tag `skill: review-process-scope`, cite `C8.x`, use `[PROC]` for process nits. Distinguish missing process context from a +demonstrated production or test-reliability problem, and use the canonical severity definitions. ## When to defer to the developer diff --git a/kb/skills/review-testing.md b/kb/skills/review-testing.md index 9adff0dbd71b..fd73eff230c6 100644 --- a/kb/skills/review-testing.md +++ b/kb/skills/review-testing.md @@ -1,54 +1,52 @@ # review-testing -You are a specialized reviewer for **Apache Pinot domain 6: Testing Strategies**. Read `kb/code-review-principles.md` section 6 and `CLAUDE.md`. +Review **Apache Pinot domain 6: Testing Strategies**. Read the applicable parts of section 6 in +`kb/code-review-principles.md` and relevant repository conventions not already loaded. Reuse material already read. -Severity: -- **CRITICAL** — bug-fix PR with no regression test; wire-format change with no mixed-version test; null-aware change that tests only one of the two null-handling modes. -- **MAJOR** — positive-only test (no negative / error-case); mock-heavy test where a real dictionary/segment is trivially available; missing test for a new public code path; new integration test spins up its own cluster when `CustomDataQueryClusterIntegrationTest` would suffice. -- **MINOR** — assertion style (`assertTrue(x == y)` vs `assertEquals`); unclear test names; flaky-prone timing assumptions. +Use the canonical severity definitions and Review Delivery rules in `kb/code-review-principles.md`. Assess demonstrated +impact; pattern matches are investigation triggers, not findings or automatic severity assignments. ## 1. Broad scan -- For every file under `src/main/` changed, check if a corresponding `src/test/` file also changed. Missing = suspect. +- Map changed behavior to existing and new tests. A production-file change without a test-file change is not itself a + coverage gap; identify the behavior or failure mode that lacks coverage. - Find new tests and scan for: - Test framework: TestNG (`import org.testng.annotations.Test`) unless the file uses JUnit consistently. - - Mocks: `@Mock`, `Mockito.when`, `mock(...)`. Flag mocks of `Dictionary`, `ForwardIndexReader`, `NullValueVectorReader` — these should be real in most cases (see PR 18189). + - Mocks: inspect whether mocks of `Dictionary`, `ForwardIndexReader`, or `NullValueVectorReader` omit semantics needed + by the assertion. Prefer real instances when encoding or storage behavior matters (see PR 18189). - `assertTrue` / `assertFalse` on compound expressions — prefer `assertEquals` / `assertThrows`. - - Missing `@Test(dataProvider=...)` when the production code has a type-dispatch switch — a single type is insufficient coverage. - - Timing assumptions: `Thread.sleep`, `System.currentTimeMillis()` in assertions → flakiness. -- Integration tests: new REST / Thrift / wire-format changes need a `*IntegrationTest` case; check `pinot-integration-tests/`. -- Integration-test base class: reject new standalone Pinot integration test classes that use `BaseClusterIntegrationTest` - or spin up their own controller/broker/server when no special cluster setup is required. For ordinary schema/data/query - behavior, require reusing an existing `CustomDataQueryClusterIntegrationTest` test or adding a focused subclass under - `pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/` with - `@Test(suiteName = "CustomClusterIntegrationTest")`, so it is included by - `pinot-integration-tests/src/test/resources/custom-cluster-integration-test-suite.xml`. + - Type-dispatch coverage: confirm the affected branches are exercised, through data providers or other focused cases. + - Timing assumptions: inspect `Thread.sleep` and wall-clock assertions for dependence on uncontrolled scheduling. +- Integration tests: check existing coverage of changed REST / Thrift / wire behavior and identify gaps at the affected + component boundary. See C6.4 and C6.10 for full-pipeline coverage and shared-cluster selection. ## 2. Deep analysis -- **C6.x** Positive + negative: every new feature needs at least one passing and one rejected-input test. -- **Real dependencies where semantics matter**: dictionaries, segment readers, null-vector readers, aggregators, transform functions — use real instances. Mocks hide encoding-sensitive bugs. -- **Null-handling coverage**: any change under null-aware code must test both `null-handling-enabled=true` and `false`. +- **C6.x** Positive + negative: check successful behavior and relevant boundary or rejected-input cases. Reuse existing + coverage; do not invent an error contract just to meet a test-count rule. +- **Real dependencies where semantics matter**: check that dictionaries, segment readers, null-vector readers, + aggregators, and transform functions preserve the semantics the test claims to validate. +- **Null-handling coverage**: check both `null-handling-enabled=true` and `false` when the change affects both paths. - **Type coverage**: for aggregator / window / transform changes, test INT, LONG, FLOAT, DOUBLE, BIG_DECIMAL, STRING, BYTES, BOOLEAN, TIMESTAMP, JSON as applicable. -- **Mixed-version tests**: for wire-format, Helix, or controller-API changes, confirm a rolling-upgrade scenario is exercised (controller-old + broker-new, and vice-versa). -- **Regression evidence**: bug-fix PRs must include a test that fails on `HEAD~1` and passes on `HEAD`. Flag absence. +- **Mixed-version tests**: for changes affecting rolling-upgrade compatibility, check the supported old/new combinations + at the changed boundary. Reuse applicable compatibility evidence and identify any missing scenario. +- **Regression evidence**: verify evidence that the regression test fails without the fix and passes with it. Use the + actual pre-fix revision or a controlled removal of the fix, not an assumed `HEAD~1` baseline. Do not change the checkout + or run tests solely to satisfy a checklist; use safe, scoped verification appropriate to the task and report material + validation gaps. - **Flakiness hygiene**: never add a `Thread.sleep` as a test-stability knob; prefer `Awaitility` / explicit events. Do not mask flakes with retries (see domain 8 process rule). ### Core-functionality + integration-test base-class selection -Every non-trivial change must exercise the **core functionality** it introduces. For query-semantics changes (new function, index type, aggregator, transform, SQL construct, stored-type behavior) that can be validated with ordinary table data and the default cluster topology, the integration test **must** reuse an existing `CustomDataQueryClusterIntegrationTest` test or add a focused subclass under `pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/` — not a fresh `BaseClusterIntegrationTest` subclass. +For new or changed query semantics, check coverage at the smallest layer that proves the behavior under C6.4. Require +integration coverage only when correctness depends on pipeline or component interactions that focused tests do not cover. +Existing tests may provide that evidence; add a finding only for a concrete coverage gap. -Rationale: `CustomDataQueryClusterIntegrationTest` shares one controller / broker / server / ZK across the whole test suite (`@BeforeSuite`, `_sharedClusterTestSuite`). Spinning up a second cluster per test costs ~30–60 s of ZK/Helix startup and inflates CI time linearly with test count. The custom base lets each test bring its own schema, data, and SQL assertions on top of the shared cluster. Nearly all feature validation (window functions, sketches, vector indexes, geo, JSON, timestamp, bytes, distinct, group-by options, star-tree, unnest, SSB queries, etc.) already follows this pattern — look for neighbors in `pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/` before adding a new top-level cluster test. - -A new test may extend `BaseClusterIntegrationTest` (or a specialized base) **only** when the change demands one of: -- Different cluster topology (multi-tenant, multi-broker, multi-server, dedicated minion). -- Non-default Pinot component configuration (custom broker / server / controller properties, auth, TLS, access control). -- Different Helix / ZK layout or tenant isolation that can't be simulated with table-level config. -- Realtime/streaming wiring that the custom base does not already provide, or lifecycle transitions that require cluster restart. - -Flag as **MAJOR / C6.10**: a new test under `pinot-integration-tests/` whose class body only defines schema, data, and SQL assertions but extends `BaseClusterIntegrationTest` directly or otherwise creates a dedicated cluster. The fix is to re-parent to `CustomDataQueryClusterIntegrationTest`, move it into the `custom/` package, annotate it with `@Test(suiteName = "CustomClusterIntegrationTest")`, and ensure it is covered by `custom-cluster-integration-test-suite.xml`. Require the PR to state, in the description, which of the four "only-when" conditions above applies if the fresh cluster is justified. - -Independently, flag as **CRITICAL** a change to core functionality (new SQL function, new index type, behavior change of an existing operator/aggregator) that ships with only unit tests and no integration test of any kind — unit coverage alone doesn't prove the feature works end-to-end through planner → broker → server → segment. +When integration coverage is needed for ordinary table/data/query scenarios, apply C6.10 and reuse +`CustomDataQueryClusterIntegrationTest`. Follow its canonical +package, suite annotation, and suite-discovery guidance. A separate cluster needs a distinct topology, component +configuration, isolation, streaming setup, or lifecycle requirement that the shared suite cannot support. Establish that +need from the code or supplied design context; do not require duplicate PR prose when the reason is already clear. ## 3. Findings diff --git a/kb/skills/run-test.md b/kb/skills/run-test.md index 3648fc3ac704..7231090d6797 100644 --- a/kb/skills/run-test.md +++ b/kb/skills/run-test.md @@ -5,16 +5,16 @@ Purpose: resolve a test class name to its Maven module and run only that test, w Usage: - `/run-test RangeIndexTest` — single class. - `/run-test RangeIndexTest#testSpecificMethod` — single method. -- `/run-test OfflineClusterIntegrationTest` — integration test (auto-detected, adds the required flag). +- `/run-test OfflineClusterIntegrationTest` — integration test (auto-detected for runtime expectations). ## Procedure -1. **Parse the argument.** Split on `#` into `<className>` and optional `<methodName>`. If the class name contains a dot, treat it as FQN. +1. **Parse the argument.** Split on `#` into `<className>` and optional `<methodName>`. If the class name contains a dot, retain it as the FQN selector and extract its simple class name for file lookup. 2. **Locate the source file.** - - Glob for `**/<className>.java` under the repo. + - Glob for `**/<simpleClassName>.java` under the repo; verify the package declaration when the user supplied an FQN. - Prefer matches under `src/test/java/`. - - If multiple matches, list them (with module prefixes) and ask the user which one. Do not guess. + - Use the supplied module or established task scope to disambiguate matches. Ask with module-qualified candidates only if multiple matches remain. - If zero matches, report and stop. 3. **Find the owning module.** Walk up from the test file until you find a `pom.xml` that is not the repo root. That's the module. @@ -30,17 +30,19 @@ Usage: 5. **Build the command.** ``` - ./mvnw -pl <module> -am -Dtest=<className>[#<methodName>] -Dsurefire.failIfNoSpecifiedTests=false test + ./mvnw -pl <module> -am '-Dtest=<selector>' -Dsurefire.failIfNoSpecifiedTests=false test ``` - - `-am` is intentional: the test needs upstream module JARs built. + - Use the class name or FQN as `<selector>`; append `#<methodName>` when a method is requested. + - Keep `-am` when current upstream dependencies are unverified. For module-only iteration, omit it only when the required artifacts are available to Maven and verified against the current source, dependency versions, JDK, and build configuration. A prior reactor `test` alone does not establish that dependency artifacts are installed for a module-only invocation. - `-Dsurefire.failIfNoSpecifiedTests=false` is always required when `-am` is set (see step 4). -6. **Run and report.** Print the exact command before running so the user can copy/tweak it. On failure, show the last ~60 lines of the Maven output (or the Surefire report path under `<module>/target/surefire-reports/`) so the user can jump straight to the stack trace. +6. **Run and report.** Print the exact command before running so the user can copy/tweak it. Long runs may execute asynchronously; retain the log and Maven exit code, report meaningful progress, and do independent work while they run. Await completion and verify that the requested test actually ran before reporting success. On failure, show the relevant Maven output or the Surefire report path under `<module>/target/surefire-reports/`. ## Notes -- These runs can take 2–15 minutes depending on the module and whether deps are already built. Consider `run_in_background` only if the user says so — default is foreground so they see progress. -- Never strip `-am`. The first run after a clean checkout will fail without it. -- If the user wants to run without rebuilding upstream (faster iteration), suggest they add `-o` (offline) or drop `-am` after the first successful build — but don't do it automatically. -- For repeat runs of the same test, suggest `-DfailIfNoTests=false` if the first run reported "No tests were executed" — usually a typo in the class name. -- If the class is `abstract` or has no `@Test` methods (it's a base class), warn the user and suggest concrete subclasses found via grep. +- These runs can take 2–15 minutes depending on the module and dependency state. Asynchronous execution does not require separate user authorization. +- `-o` disables remote artifact resolution; it does not skip upstream reactor modules or replace `-am`. Use it only when all required artifacts are cached and offline execution is appropriate. +- Reuse a passing test result if the tested sources, dependencies, JDK, and relevant configuration have not changed. Rerun only when changes or unresolved failures invalidate that evidence. +- If the requested test did not execute, check the selector, source class, and Surefire reports. Do not suppress the missing-test failure to report success. +- If the class is abstract, suggest concrete subclasses. Before treating a class with no declared `@Test` methods as a base class, check inherited tests and class-level test annotations. +- Integration tests can start embedded Helix/ZK/Kafka and bind fixed localhost ports. Do not overlap runs that share ports or other exclusive resources.