terrymanu commented on PR #39031: URL: https://github.com/apache/shardingsphere/pull/39031#issuecomment-4902316486
### Summary **Review Result: Mergeable** Reason: The latest PR head is code-scope ready for this Code Correctness Review. The change is limited to replacing boilerplate accessor methods with narrow Lombok annotations, and I did not find a root-cause, compatibility, visibility, or regression blocker. CI not reviewed by request. ### Evidence - The PR scope is narrow and matches the stated intent: 14 changed files, all replacing hand-written getter/setter boilerplate with Lombok annotations, with no linked issue or broader behavior claim. - `parser/sql/statement/core/src/main/java/org/apache/shardingsphere/sql/parser/statement/core/segment/ddl/partition/AddPartitionDefinitionSegment.java:33` keeps the existing constructor/getter Lombok pattern and adds `@Setter`; the nullable fields remain the only mutable fields at lines 44-50, while the Optional-returning getters remain explicit. - `proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/blob/upload/FirebirdBlobUpload.java:25` already uses class-level `@Getter`; removing the manual `isClosed()` leaves the same boolean accessor generated for the `closed` field at line 34, and `markClosed()` remains unchanged at lines 59-63. - `infra/common/src/main/java/org/apache/shardingsphere/infra/metadata/identifier/IdentifierIndex.java:330` and lines 355-370 use private Lombok getters for private nested state only, preserving the previous internal visibility. - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/runtime/ReadinessProbe.java:165` keeps the private constructor and private accessor visibility for `ReadinessResult`, while the public factory methods at lines 184, 195, and 206 remain unchanged. ### Review Details - Review Focus: Code Correctness Review; CI not reviewed by request. - Reviewed Scope: all 14 files from GitHub `/pulls/39031/files`; latest PR head `8ffd81685cda6e97e5ee6079e481f9459950d60b`; base and local merge-base `c766f879a08513d51f1d87b160d09b015c8ecf64`; local triple-dot file list matched GitHub exactly. - Not Reviewed Scope: GitHub Actions/check-runs and full runtime E2E execution were not reviewed. No review threads or PR comments existed at the time of review. - Verification: `./mvnw spotless:check -Pcheck -T1C` exit 0; `./mvnw checkstyle:check -Pcheck -T1C` exit 0; `./mvnw -pl infra/common,mcp/core,mcp/features/sharding,mcp/support,mode/core,parser/sql/statement/core,proxy/frontend/dialect/firebird,test/e2e/mcp,test/e2e/sql -am -DskipTests test-compile -Pcheck -T1C` exit 0. - Release Note / User Docs: not required; this is an internal boilerplate accessor refactor with no user-facing behavior, configuration, SQL syntax, protocol, or migration impact. -- 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]
