-
Notifications
You must be signed in to change notification settings - Fork 133
Added the compact api for indexes #901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added the compact api for indexes #901
Conversation
WalkthroughAdds index compaction support: a new public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Integration Test
participant SDK as Index.compact()
participant API as Meilisearch API
Note over SDK,API: Compact request flow (new)
Test->>SDK: call compact()
SDK->>API: POST /indexes/{uid}/compact
API-->>SDK: 202 Accepted (TaskInfo)
SDK-->>Test: returns TaskInfo (task uid)
Note right of Test: Test polls tasks endpoint to observe status changes
Test->>API: GET /tasks/{taskUid}
API-->>Test: {status: enqueued -> succeeded, type: indexCompaction}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/meilisearch/sdk/Index.java (1)
1298-1312: Consider adding leading slash for consistency.The implementation looks correct overall. However, at line 1309,
addSubroute("compact")is called without a leading slash, while a similar method at line 1256 usesaddSubroute("/similar")with a leading slash. While URLBuilder may handle both patterns, maintaining consistency improves code readability and maintainability.Apply this diff for consistency:
public TaskInfo compact() throws MeilisearchException { return this.config.httpClient.post( - new URLBuilder("/indexes").addSubroute(this.uid).addSubroute("compact").getURL(), + new URLBuilder("/indexes").addSubroute(this.uid).addSubroute("/compact").getURL(), null, TaskInfo.class); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.code-samples.meilisearch.yaml(1 hunks)src/main/java/com/meilisearch/sdk/Index.java(1 hunks)src/test/java/com/meilisearch/integration/ClientTest.java(1 hunks)
🔇 Additional comments (2)
.code-samples.meilisearch.yaml (1)
855-856: LGTM!The code sample correctly demonstrates the compact API usage and follows the same pattern as other samples in this file.
src/test/java/com/meilisearch/integration/ClientTest.java (1)
349-378: LGTM!The test provides comprehensive coverage of the compact API:
- Creates an index and adds documents
- Invokes the compact operation
- Validates task status transitions (ENQUEUED → SUCCEEDED)
- Confirms task type is "indexCompaction"
- Verifies data integrity post-compaction
The test follows the established patterns in this file and includes all necessary assertions.
|
Hi @panchasarapavan, and thank you for your PR 🙌 Can you look into the failing tests? If that helps, |
|
@Strift I am looking into them. I had ran them locally. Will check why they are failing here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/com/meilisearch/integration/ClientTest.java (1)
349-378: Consider adding a test case for compacting an empty index.The current test validates compaction with documents. Adding a complementary test for compacting an empty index would improve coverage and ensure the API handles both scenarios gracefully.
Example test structure:
@Test public void testCompactEmptyIndex() throws Exception { String indexUid = "testCompactEmptyIndex"; Index index = createEmptyIndex(indexUid, this.primaryKey); TaskInfo compactTask = index.compact(); client.waitForTask(compactTask.getTaskUid()); Task completedCompactTask = client.getTask(compactTask.getTaskUid()); assertThat(compactTask.getStatus(), is(equalTo(TaskStatus.ENQUEUED))); assertThat(completedCompactTask.getType(), is(equalTo("indexCompaction"))); assertThat(completedCompactTask.getStatus(), is(equalTo(TaskStatus.SUCCEEDED))); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/meilisearch/sdk/Index.java(1 hunks)src/test/java/com/meilisearch/integration/ClientTest.java(1 hunks)
🔇 Additional comments (2)
src/main/java/com/meilisearch/sdk/Index.java (1)
1299-1311: LGTM! Well-implemented compact API.The
compact()method is correctly implemented following the established patterns in the codebase. The Javadoc is clear, the endpoint construction is accurate, and the method signature is consistent with other asynchronous operations that returnTaskInfo.src/test/java/com/meilisearch/integration/ClientTest.java (1)
349-378: LGTM! Comprehensive integration test for index compaction.The test is well-structured and follows existing patterns in the codebase. It correctly validates the compact operation by checking task status, task type, and data integrity post-compaction.
|
@Strift i have updated the pr. Fixed the failing tests. |
|
Thanks for the swift update! Let's get this merged 🚀 |
Pull Request
Related issue
Fixes #900
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Tests