yuqi1129 commented on code in PR #11339:
URL: https://github.com/apache/gravitino/pull/11339#discussion_r3338540803
##########
AGENTS.md:
##########
@@ -42,10 +42,54 @@
4. Constructors.
5. Methods (Group by visibility, putting `private` methods at the end).
+## Code Design & Structure
+- **Method granularity**: Each method must earn its existence. Avoid small,
fragmented single-use helpers — keep cohesive logic together. At the same time,
fold genuinely duplicated logic into one shared helper instead of repeating it.
Extract when it is reused or names a genuinely non-obvious step; inline trivial
one-liners.
+- **No dead parameters or methods**: Remove a parameter that is always the
same constant (for example, a flag that is always `true`) and a method that
does nothing meaningful. Keep related declarations and their setup together
(for example, a thread-pool field and the loop that submits tasks to it).
+- **Field assignment**: Qualify field assignments with `this.` (for example,
`this.workers = ...`) in constructors and setters.
+- **Test-only code must not live in production classes**: Never add a
production method or field that exists only to serve a test.
+ - Prefer keeping test-only logic in the test module (test helpers, fixtures,
or the test class itself).
+ - If a production member genuinely must be reachable from a test, annotate
it with `@VisibleForTesting` and keep the narrowest visibility possible
(package-private, not `public`/`protected`). Never widen visibility solely for
a test.
+ - Do not reach into non-public members from tests via reflection
(`setAccessible(true)`). Add a `@VisibleForTesting` constructor/factory instead.
+- **Narrowest visibility**: Give every class member the smallest visibility
that works (`private` first, then package-private). Do not make something
`public`/`protected` unless an external caller needs it.
+- **Dependency injection, not singletons**: Avoid the singleton pattern
(including lazy-holder/double-checked instances) — it is hard to test.
Construct collaborators once in the owning class (for example, the
plugin/bootstrap class) and inject them; do not `new` a manager/service inline
in the middle of logic, and never create two instances of a component that is
meant to be shared.
+- **Naming follows existing conventions**: Match the naming patterns already
used by sibling classes and the surrounding package, and make the name reflect
the class's actual role. Established suffixes include `XxxManager`,
`XxxDispatcher`, `XxxService`, `XxxOperations`, `XxxListener`, `XxxPoller`,
capability interfaces `SupportsXxx`, and tests `TestXxx`. Capitalize acronyms
consistently with existing code (`RESTUtils`, not `RestUtils`). Name boolean
fields without an `is` prefix (`basicAuthEnabled`, not `isBasicAuthEnabled`).
Do not invent a new convention when a matching one already exists.
+- **Reuse existing types and patterns**: Before adding a new request DTO, PO,
or helper, check whether an existing one (for example, an `XxxAssociateRequest`
or `XxxPO`) already fits, and reuse it. Validate request DTOs at the request
boundary (reject nulls, overlapping add/remove sets, etc.).
+- **Exceptions**: Throw the standard, semantically correct exception type for
the situation (for example, the project's unauthorized/forbidden exception for
auth failures). Do not add a `catch (RuntimeException e) { throw e; }` that
adds no behavior, and collapse identical sibling `catch` blocks. When
refactoring, preserve identifying context (such as the table identifier) in
error messages and logs.
+- **Prefer standard-library and Commons idioms**: Use
`StringUtils.isBlank`/`isNotBlank` instead of manual null/empty string checks,
a `Set` (not a `List`) for repeated membership lookups, and
`Base64.getEncoder().encodeToString(...)` rather than manual byte/string
round-trips. Do not use deprecated APIs (for example,
`RandomStringUtils.randomAlphabetic`); keep a
`@SuppressWarnings("deprecation")` only when it is required for compatibility,
with a comment explaining why.
+- **Component placement & lifecycle**: Put a class in the module that owns its
concern. A sub-component of another component (for example, a side module of
the entity store) should be created, started, and closed by its owner —
mirroring sibling components such as `RelationalGarbageCollector` — rather than
being exposed and wired as a global `GravitinoEnv` component.
+- **Prefer capability interfaces over concrete casts**: Detect optional
behavior through a `SupportsXxx` capability interface plus `instanceof`, not by
casting to a concrete implementation class.
+- **Argument validation**: Use `Preconditions.checkArgument(condition,
message)` for argument/state validation; do not use `checkNotNull`. Only
null-check values that can actually be null — do not add defensive null checks
for values guaranteed non-null by their source (for example, rows returned by a
MyBatis mapper).
+- **Remove superseded code**: When you replace an implementation, delete the
old classes, methods, fields, and imports it leaves behind. Do not leave
orphaned or unreferenced code.
+
+## Configuration Changes
+- When adding a new `ConfigEntry`, document it in the matching table in
`docs/gravitino-server-config.md` (config name, description, default value,
required, and since version).
+- Any test that initializes the owning component with a **mocked `Config`**
must stub the new key. A real `Config` returns the entry's registered default,
but a Mockito-mocked `Config` returns `null` and will throw
`NullPointerException` on unboxing (for example, to `long`). Follow how
existing keys such as `STORE_DELETE_AFTER_TIME` are already stubbed.
+- Validate config values at the entry (for example, `checkValue(v -> v > 0,
...)`). Once validated, do not re-guard the value downstream (no `Math.max(1,
...)` on an already-positive value).
+
+## Concurrency & Background Tasks
+- **Lifecycle flags**: A `running`/`started` flag that can be touched
concurrently must be an `AtomicBoolean` guarded with `compareAndSet`. Make
`start()` idempotent, and make it fail fast (not run with shut-down executors)
if called after `close()`.
+- **Shared mutable state**: Guard shared mutable state and watch for TOCTOU
windows when one thread updates a shared map/token that another reads. When the
same collection is handed to multiple consumers, pass an immutable view (for
example, `Collections.unmodifiableList(...)`).
+- **Scheduled tasks must self-protect**: A `Runnable` scheduled on a
`ScheduledExecutorService` must catch and log its own exceptions — an uncaught
throwable silently cancels all future executions. Advance cursors/last-run
timestamps in a `finally` block so a transient failure does not cause
tight-loop retries, and restore the interrupt flag on `InterruptedException`.
Review Comment:
Fixed — reworded to 'cancels all future executions of that task (other
scheduled tasks keep running)'.
##########
AGENTS.md:
##########
@@ -42,10 +42,54 @@
4. Constructors.
5. Methods (Group by visibility, putting `private` methods at the end).
+## Code Design & Structure
+- **Method granularity**: Each method must earn its existence. Avoid small,
fragmented single-use helpers — keep cohesive logic together. At the same time,
fold genuinely duplicated logic into one shared helper instead of repeating it.
Extract when it is reused or names a genuinely non-obvious step; inline trivial
one-liners.
+- **No dead parameters or methods**: Remove a parameter that is always the
same constant (for example, a flag that is always `true`) and a method that
does nothing meaningful. Keep related declarations and their setup together
(for example, a thread-pool field and the loop that submits tasks to it).
+- **Field assignment**: Qualify field assignments with `this.` (for example,
`this.workers = ...`) in constructors and setters.
+- **Test-only code must not live in production classes**: Never add a
production method or field that exists only to serve a test.
+ - Prefer keeping test-only logic in the test module (test helpers, fixtures,
or the test class itself).
+ - If a production member genuinely must be reachable from a test, annotate
it with `@VisibleForTesting` and keep the narrowest visibility possible
(package-private, not `public`/`protected`). Never widen visibility solely for
a test.
+ - Do not reach into non-public members from tests via reflection
(`setAccessible(true)`). Add a `@VisibleForTesting` constructor/factory instead.
+- **Narrowest visibility**: Give every class member the smallest visibility
that works (`private` first, then package-private). Do not make something
`public`/`protected` unless an external caller needs it.
+- **Dependency injection, not singletons**: Avoid the singleton pattern
(including lazy-holder/double-checked instances) — it is hard to test.
Construct collaborators once in the owning class (for example, the
plugin/bootstrap class) and inject them; do not `new` a manager/service inline
in the middle of logic, and never create two instances of a component that is
meant to be shared.
+- **Naming follows existing conventions**: Match the naming patterns already
used by sibling classes and the surrounding package, and make the name reflect
the class's actual role. Established suffixes include `XxxManager`,
`XxxDispatcher`, `XxxService`, `XxxOperations`, `XxxListener`, `XxxPoller`,
capability interfaces `SupportsXxx`, and tests `TestXxx`. Capitalize acronyms
consistently with existing code (`RESTUtils`, not `RestUtils`). Name boolean
fields without an `is` prefix (`basicAuthEnabled`, not `isBasicAuthEnabled`).
Do not invent a new convention when a matching one already exists.
+- **Reuse existing types and patterns**: Before adding a new request DTO, PO,
or helper, check whether an existing one (for example, an `XxxAssociateRequest`
or `XxxPO`) already fits, and reuse it. Validate request DTOs at the request
boundary (reject nulls, overlapping add/remove sets, etc.).
+- **Exceptions**: Throw the standard, semantically correct exception type for
the situation (for example, the project's unauthorized/forbidden exception for
auth failures). Do not add a `catch (RuntimeException e) { throw e; }` that
adds no behavior, and collapse identical sibling `catch` blocks. When
refactoring, preserve identifying context (such as the table identifier) in
error messages and logs.
+- **Prefer standard-library and Commons idioms**: Use
`StringUtils.isBlank`/`isNotBlank` instead of manual null/empty string checks,
a `Set` (not a `List`) for repeated membership lookups, and
`Base64.getEncoder().encodeToString(...)` rather than manual byte/string
round-trips. Do not use deprecated APIs (for example,
`RandomStringUtils.randomAlphabetic`); keep a
`@SuppressWarnings("deprecation")` only when it is required for compatibility,
with a comment explaining why.
Review Comment:
Fixed — dropped the RandomStringUtils.randomAlphabetic example (it is not
deprecated at the pinned commons-lang3 3.14.0) and stated the rule generically.
##########
AGENTS.md:
##########
@@ -42,10 +42,54 @@
4. Constructors.
5. Methods (Group by visibility, putting `private` methods at the end).
+## Code Design & Structure
+- **Method granularity**: Each method must earn its existence. Avoid small,
fragmented single-use helpers — keep cohesive logic together. At the same time,
fold genuinely duplicated logic into one shared helper instead of repeating it.
Extract when it is reused or names a genuinely non-obvious step; inline trivial
one-liners.
+- **No dead parameters or methods**: Remove a parameter that is always the
same constant (for example, a flag that is always `true`) and a method that
does nothing meaningful. Keep related declarations and their setup together
(for example, a thread-pool field and the loop that submits tasks to it).
+- **Field assignment**: Qualify field assignments with `this.` (for example,
`this.workers = ...`) in constructors and setters.
+- **Test-only code must not live in production classes**: Never add a
production method or field that exists only to serve a test.
+ - Prefer keeping test-only logic in the test module (test helpers, fixtures,
or the test class itself).
+ - If a production member genuinely must be reachable from a test, annotate
it with `@VisibleForTesting` and keep the narrowest visibility possible
(package-private, not `public`/`protected`). Never widen visibility solely for
a test.
+ - Do not reach into non-public members from tests via reflection
(`setAccessible(true)`). Add a `@VisibleForTesting` constructor/factory instead.
+- **Narrowest visibility**: Give every class member the smallest visibility
that works (`private` first, then package-private). Do not make something
`public`/`protected` unless an external caller needs it.
+- **Dependency injection, not singletons**: Avoid the singleton pattern
(including lazy-holder/double-checked instances) — it is hard to test.
Construct collaborators once in the owning class (for example, the
plugin/bootstrap class) and inject them; do not `new` a manager/service inline
in the middle of logic, and never create two instances of a component that is
meant to be shared.
+- **Naming follows existing conventions**: Match the naming patterns already
used by sibling classes and the surrounding package, and make the name reflect
the class's actual role. Established suffixes include `XxxManager`,
`XxxDispatcher`, `XxxService`, `XxxOperations`, `XxxListener`, `XxxPoller`,
capability interfaces `SupportsXxx`, and tests `TestXxx`. Capitalize acronyms
consistently with existing code (`RESTUtils`, not `RestUtils`). Name boolean
fields without an `is` prefix (`basicAuthEnabled`, not `isBasicAuthEnabled`).
Do not invent a new convention when a matching one already exists.
+- **Reuse existing types and patterns**: Before adding a new request DTO, PO,
or helper, check whether an existing one (for example, an `XxxAssociateRequest`
or `XxxPO`) already fits, and reuse it. Validate request DTOs at the request
boundary (reject nulls, overlapping add/remove sets, etc.).
+- **Exceptions**: Throw the standard, semantically correct exception type for
the situation (for example, the project's unauthorized/forbidden exception for
auth failures). Do not add a `catch (RuntimeException e) { throw e; }` that
adds no behavior, and collapse identical sibling `catch` blocks. When
refactoring, preserve identifying context (such as the table identifier) in
error messages and logs.
+- **Prefer standard-library and Commons idioms**: Use
`StringUtils.isBlank`/`isNotBlank` instead of manual null/empty string checks,
a `Set` (not a `List`) for repeated membership lookups, and
`Base64.getEncoder().encodeToString(...)` rather than manual byte/string
round-trips. Do not use deprecated APIs (for example,
`RandomStringUtils.randomAlphabetic`); keep a
`@SuppressWarnings("deprecation")` only when it is required for compatibility,
with a comment explaining why.
+- **Component placement & lifecycle**: Put a class in the module that owns its
concern. A sub-component of another component (for example, a side module of
the entity store) should be created, started, and closed by its owner —
mirroring sibling components such as `RelationalGarbageCollector` — rather than
being exposed and wired as a global `GravitinoEnv` component.
+- **Prefer capability interfaces over concrete casts**: Detect optional
behavior through a `SupportsXxx` capability interface plus `instanceof`, not by
casting to a concrete implementation class.
+- **Argument validation**: Use `Preconditions.checkArgument(condition,
message)` for argument/state validation; do not use `checkNotNull`. Only
null-check values that can actually be null — do not add defensive null checks
for values guaranteed non-null by their source (for example, rows returned by a
MyBatis mapper).
Review Comment:
Keeping the rule: we intentionally disallow Preconditions.checkNotNull
because it throws NullPointerException rather than the more meaningful
IllegalArgumentException. Added that rationale. Existing usages (e.g. EventBus)
are pre-existing and discouraged going forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
