refactor(biz): 修改提交查询逻辑以使用ID替代时间戳#5584
Closed
youngzil wants to merge 2 commits intoapolloconfig:masterfrom
Closed
Conversation
dosubot
bot
added
the
size:M
Contributor
📝 WalkthroughWalkthroughThe commit filtering logic is refactored from timestamp-based filtering to ID-based filtering across the repository, service, and controller layers. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as ItemController
participant Service as CommitService
participant Repository as CommitRepository
participant DB as Database
Client->>Controller: findDeletedItems()
Controller->>Controller: getLatestActiveRelease()
Controller->>Service: find(appId, clusterName, namespace, releaseId)
Service->>Repository: findByAppIdAndClusterNameAndNamespaceNameAndIdGreaterThanEqualOrderByIdDesc(...)
Repository->>DB: Query commits WHERE id >= releaseId
DB-->>Repository: Return commits
Repository-->>Service: List<Commit>
Service-->>Controller: List<Commit>
Controller->>Controller: Filter deleted items
Controller-->>Client: Deleted items
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java (1)
127-141: Test coverage gap: Does not validate the actual use case.This test validates the
commitService.find(..., id, ...)method using Commit IDs, which is correct for the service layer. However, it doesn't cover theItemController.findDeletedItems()scenario where a Release ID is passed to filter Commits—the semantic mismatch flagged inItemController.java.Consider adding an integration test that mirrors the controller's usage pattern to catch the cross-entity ID comparison issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java` around lines 127 - 141, Add an integration test that reproduces the controller path: create a Release (use the Release entity or releaseService to persist it), then call the same code path as ItemController.findDeletedItems (or invoke the controller method directly) so the filtering uses a Release ID; verify the returned Commits/Items are filtered by releaseId and not by commit.id to catch the cross-entity ID comparison bug. Locate this new test alongside NamespaceServiceIntegrationTest (or extend it), and exercise commitService.find and ItemController.findDeletedItems to assert correct behavior when passing a releaseId. Ensure you create distinct commit and release records so the test will fail if the code compares commit.id to the releaseId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java`:
- Around line 205-206: The code incorrectly filters commits by comparing
commit.id >= latestActiveRelease.getId() (see commitService.find call and use of
latestActiveRelease.getId()), but Release.id and Commit.id are independent
sequences; instead change to a timestamp-based filter: use
latestActiveRelease.getDataChangeCreatedTime() to query commits (or first query
Commit to find the minimal commit ID where dataChangeCreatedTime >=
latestActiveRelease.getDataChangeCreatedTime() and then pass that commitId into
commitService.find), and/or add an index on Commit.DataChange_LastTime to
preserve performance; update the call sites and commitService.find
signature/implementation as needed to accept a timestamp or startingCommitId
(referencing commitService.find and
latestActiveRelease.getDataChangeCreatedTime) so commits are selected by time
correlation rather than cross-entity IDs.
In
`@apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java`:
- Around line 124-141: The test method name and formatting need fixing: run the
formatter (mvn spotless:apply or ./mvnw spotless:apply) and commit the changes,
then rename the test method
NamespaceServiceIntegrationTest.testGetCommitsByModifiedTime to a name that
matches its behavior (e.g., testGetCommitsByIdFilter or
testFindCommitsByIdThreshold) and update any references to that method; ensure
the method body (calls to commitService.find and assertions) remains unchanged
and that the class compiles after the rename.
---
Nitpick comments:
In
`@apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java`:
- Around line 127-141: Add an integration test that reproduces the controller
path: create a Release (use the Release entity or releaseService to persist it),
then call the same code path as ItemController.findDeletedItems (or invoke the
controller method directly) so the filtering uses a Release ID; verify the
returned Commits/Items are filtered by releaseId and not by commit.id to catch
the cross-entity ID comparison bug. Locate this new test alongside
NamespaceServiceIntegrationTest (or extend it), and exercise commitService.find
and ItemController.findDeletedItems to assert correct behavior when passing a
releaseId. Ensure you create distinct commit and release records so the test
will fail if the code compares commit.id to the releaseId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 042ae06d-4170-412e-a099-8d933b48b307
📒 Files selected for processing (5)
.gitignoreapollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.javaapollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/CommitRepository.javaapollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/CommitService.javaapollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java
Comment on lines
+205
to
+206
| commits = | ||
| commitService.find(appId, clusterName, namespaceName, latestActiveRelease.getId(), null); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that Release and Commit entities both extend BaseEntity with independent ID sequences
echo "=== Checking Release entity ==="
ast-grep --pattern 'class Release extends BaseEntity'
echo "=== Checking Commit entity ==="
ast-grep --pattern 'class Commit extends BaseEntity'
echo "=== Checking BaseEntity ID generation strategy ==="
rg -nA3 '@GeneratedValue' --glob '**/BaseEntity.java'Repository: apolloconfig/apollo
Length of output: 1145
🏁 Script executed:
#!/bin/bash
# Find Release and Commit entity files
echo "=== Finding Release entity ==="
fd -e java -x grep -l 'class Release' {} +
echo -e "\n=== Finding Commit entity ==="
fd -e java -x grep -l 'class Commit' {} +
echo -e "\n=== Searching for Release class definition ==="
rg -A5 'class Release' --glob '**/*.java' -m 1
echo -e "\n=== Searching for Commit class definition ==="
rg -A5 'class Commit' --glob '**/*.java' -m 1Repository: apolloconfig/apollo
Length of output: 84254
🏁 Script executed:
#!/bin/bash
# View Release entity class
echo "=== Release.java ==="
cat -n apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/entity/Release.java | head -50
# View Commit entity class
echo -e "\n=== Commit.java ==="
cat -n apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/entity/Commit.java | head -50
# Find the CommitService.find method
echo -e "\n=== CommitService.find method ==="
rg -A20 'public.*find' apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/service/CommitService.java
# View ItemController around lines 205-206
echo -e "\n=== ItemController.java lines 200-210 ==="
cat -n apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java | sed -n '200,210p'Repository: apolloconfig/apollo
Length of output: 5608
🏁 Script executed:
#!/bin/bash
# Examine the CommitRepository to understand the actual query
echo "=== CommitRepository findByAppIdAndClusterNameAndNamespaceNameAndIdGreaterThanEqualOrderByIdDesc ==="
rg -B5 -A10 'IdGreaterThanEqualOrderByIdDesc' apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/repository/CommitRepository.javaRepository: apolloconfig/apollo
Length of output: 1027
Critical: Cross-entity ID comparison produces incorrect results.
Release.id and Commit.id are generated from independent auto-increment sequences (both entities extend BaseEntity which uses GenerationType.IDENTITY). Filtering commits where commit.id >= release.id has no meaningful correlation to the release timing.
For example, if Release has ID 5000 but relevant Commit records have IDs 100-200, this query will return zero results even though those commits occurred after the release.
The original timestamp-based approach was semantically correct. To fix the performance issue, consider:
- Add an index on
Commit.DataChange_LastTimeand keep the timestamp-based query, or - Store a reference (e.g., store
lastCommitIdon Release at release time and filter using that), or - Query the Commit table to find the minimum Commit ID with
dataChangeCreatedTime >= latestActiveRelease.getDataChangeCreatedTime(), then use that Commit ID for subsequent filtering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apollo-adminservice/src/main/java/com/ctrip/framework/apollo/adminservice/controller/ItemController.java`
around lines 205 - 206, The code incorrectly filters commits by comparing
commit.id >= latestActiveRelease.getId() (see commitService.find call and use of
latestActiveRelease.getId()), but Release.id and Commit.id are independent
sequences; instead change to a timestamp-based filter: use
latestActiveRelease.getDataChangeCreatedTime() to query commits (or first query
Commit to find the minimal commit ID where dataChangeCreatedTime >=
latestActiveRelease.getDataChangeCreatedTime() and then pass that commitId into
commitService.find), and/or add an index on Commit.DataChange_LastTime to
preserve performance; update the call sites and commitService.find
signature/implementation as needed to accept a timestamp or startingCommitId
(referencing commitService.find and
latestActiveRelease.getDataChangeCreatedTime) so commits are selected by time
correlation rather than cross-entity IDs.
Comment on lines
+124
to
+141
| public void testGetCommitsByModifiedTime() { | ||
| // namespace-test.sql has 1 commit for (commitTestApp, default, application) | ||
| // find(..., id, page) returns commits with id >= given id for the namespace | ||
| List<Commit> allCommits = commitService.find(commitTestApp, testCluster, | ||
| testPrivateNamespace, PageRequest.of(0, 10)); | ||
| assertEquals(1, allCommits.size()); | ||
| Long commitId = allCommits.get(0).getId(); | ||
|
|
||
| List<Commit> commitsById = commitService.find(commitTestApp, testCluster, | ||
| testPrivateNamespace, commitId, null); | ||
| List<Commit> commitsByIdGreater = commitService.find(commitTestApp, testCluster, | ||
| testPrivateNamespace, commitId + 1, null); | ||
| List<Commit> commitsByIdPage = commitService.find(commitTestApp, testCluster, | ||
| testPrivateNamespace, commitId, PageRequest.of(0, 1)); | ||
|
|
||
| assertEquals(1, commitsById.size()); | ||
| assertEquals(0, commitsByIdGreater.size()); | ||
| assertEquals(1, commitsByIdPage.size()); |
Contributor
There was a problem hiding this comment.
Fix Spotless formatting violations and rename test method.
-
Formatting: The pipeline failed due to Spotless formatting violations. Run
mvn spotless:applybefore committing. -
Misleading name: The method
testGetCommitsByModifiedTimeno longer tests timestamp-based filtering. Rename it to reflect the current behavior, e.g.,testGetCommitsByIdFilterortestFindCommitsByIdThreshold.
As per coding guidelines, "Run ./mvnw spotless:apply to format code and must be run before opening a PR".
🧰 Tools
🪛 GitHub Actions: code style check
[error] 124-131: Spotless Java formatting check failed in spotless:2.43.0:check for project apollo-biz. The following files had format violations, including incorrect line breaks/indentation in List assignments (e.g., allCommits, commitsById, commitsByIdGreater). Run 'mvn spotless:apply' to fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/service/NamespaceServiceIntegrationTest.java`
around lines 124 - 141, The test method name and formatting need fixing: run the
formatter (mvn spotless:apply or ./mvnw spotless:apply) and commit the changes,
then rename the test method
NamespaceServiceIntegrationTest.testGetCommitsByModifiedTime to a name that
matches its behavior (e.g., testGetCommitsByIdFilter or
testFindCommitsByIdThreshold) and update any references to that method; ensure
the method body (calls to commitService.find and assertions) remains unchanged
and that the class compiles after the rename.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's the purpose of this PR
将 commit 历史查询从按时间戳过滤改为按 release ID 过滤，解决因时间字段未走索引导致的慢 SQL 问题。
Which issue(s) this PR fixes:
Fixes #
Brief changelog
背景： 原先使用
latestActiveRelease.getDataChangeCreatedTime()查询 commit，DataChange_LastTime字段没有索引，查询无法有效走索引。生产环境中，当某个 Namespace 长时间未发布时（例如最后一次发布在 2024 年初），使用如此久远的lastModifiedTime进行查询会扫描大量数据，导致执行时间过长，被 DB 监控判定为慢 SQL 而杀死。改为使用latestActiveRelease.getId()过滤，可走主键索引，无论上次发布时间多久远，查询都能保持高效稳定。变更摘要：
dataChangeLastModifiedTime（无索引）改为id（主键索引）Date lastModifiedTime改为Long idlatestActiveRelease.getId()替代getDataChangeCreatedTime()testGetCommitsByModifiedTime以适配新 APIFollow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.mvn spotless:applyto format your code.CHANGESlog.Summary by CodeRabbit
Chores
Tests