From d65085cab83589d32b68e2ab5c4dbf0b9d4f005f Mon Sep 17 00:00:00 2001 From: ylakhdar Date: Thu, 29 Jan 2026 11:01:39 -0500 Subject: [PATCH 1/5] fix(PlatformClient): validate userAgents is not null --- src/main/java/com/coveo/pushapiclient/PlatformClient.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/com/coveo/pushapiclient/PlatformClient.java b/src/main/java/com/coveo/pushapiclient/PlatformClient.java index 8523a5d5..b1ed6fdf 100644 --- a/src/main/java/com/coveo/pushapiclient/PlatformClient.java +++ b/src/main/java/com/coveo/pushapiclient/PlatformClient.java @@ -616,6 +616,9 @@ public String[] getUserAgents() { } public void setUserAgents(String[] userAgents) { + if (userAgents == null) { + throw new IllegalArgumentException("User agents cannot be null"); + } if (!validUserAgents(userAgents)) { throw new IllegalArgumentException("Invalid user agents"); } From 380eb0192d73257fb9cc705db24b5613da42f2a7 Mon Sep 17 00:00:00 2001 From: ylakhdar Date: Thu, 29 Jan 2026 11:04:22 -0500 Subject: [PATCH 2/5] feat(DocumentUploadQueue): add configurable batch size infrastructure - Add constants: MAX_ALLOWED_QUEUE_SIZE (256MB), DEFAULT_QUEUE_SIZE (256MB) - Add BATCH_SIZE_PROPERTY system property for runtime configuration - Add getConfiguredBatchSize() static method for system property lookup - Add new constructor with maxQueueSize parameter - Add validation to prevent exceeding 256MB API limit - Update existing constructor to use getConfiguredBatchSize() as default Configuration priority: constructor param > system property > default (256MB) --- .sisyphus/drafts/pr-split-186.md | 124 ++++ .sisyphus/plans/pr-186-split.md | 690 ++++++++++++++++++ .../pushapiclient/DocumentUploadQueue.java | 97 ++- .../DocumentUploadQueueTest.java | 20 +- 4 files changed, 916 insertions(+), 15 deletions(-) create mode 100644 .sisyphus/drafts/pr-split-186.md create mode 100644 .sisyphus/plans/pr-186-split.md diff --git a/.sisyphus/drafts/pr-split-186.md b/.sisyphus/drafts/pr-split-186.md new file mode 100644 index 00000000..e59ec249 --- /dev/null +++ b/.sisyphus/drafts/pr-split-186.md @@ -0,0 +1,124 @@ +# Draft: PR #186 Split Strategy + +## User's Request +Split PR #186 (feat: increase batch size to 256MB and add configurable batching) into 2-4 smaller, independently mergeable PRs. + +## PR Analysis Summary + +**Total Changes**: 1368 additions, 136 deletions, 16 files +**Branch**: `feature/configurable-batch-size-256mb` (6 commits) + +### Change Categories Identified + +1. **Core Infrastructure** (foundational - everything depends on this) + - `DocumentUploadQueue.java` - Constants, `getConfiguredBatchSize()`, new constructor with maxQueueSize + - `PlatformClient.java` - Null check for `setUserAgents()` (minor fix) + +2. **Service Layer Updates** (depends on #1) + - `PushService.java` - New constructor with maxQueueSize + - `StreamService.java` - New constructor with maxQueueSize + - `UpdateStreamService.java` - New constructors with maxQueueSize + +3. **Behavioral Change: File Container Rotation** (depends on #1, #2) + - `StreamDocumentUploadQueue.java` - `setUpdateStreamService()`, `flushAndPush()`, overridden `add()` methods + - `UpdateStreamServiceInternal.java` - `createUploadAndPush()`, modified `close()` + +4. **Documentation** + - `CONFIGURATION.md` (new, 235 lines) + - `UPGRADE_NOTES.md` (new, 171 lines) + - `README.md` (24 lines added) + - `samples/UpdateStreamDocuments.java` (8 lines) + +5. **Tests** + - `DocumentUploadQueueTest.java` - Updated for explicit batch size + - `StreamDocumentUploadQueueTest.java` - Updated tests + - `StreamDocumentUploadQueueBatchingTest.java` (new, 274 lines) + - `FileContainerRotationIntegrationTest.java` (new, 225 lines) + - `UpdateStreamServiceInternalTest.java` - Updated tests + +## Key Dependency Analysis + +``` +DocumentUploadQueue (constants + getConfiguredBatchSize + constructor) + │ + ├──► PushService (uses DEFAULT_QUEUE_SIZE, new constructor) + ├──► StreamService (uses getConfiguredBatchSize(), new constructor) + └──► UpdateStreamService (uses getConfiguredBatchSize(), new constructor) + │ + └──► StreamDocumentUploadQueue (extends DocumentUploadQueue) + │ + └──► UpdateStreamServiceInternal (uses queue.flushAndPush()) +``` + +## Technical Constraints + +1. **`DocumentUploadQueue` changes are foundational** - Services reference: + - `DEFAULT_QUEUE_SIZE` (constant) + - `getConfiguredBatchSize()` (static method) + - Constructor with `maxQueueSize` parameter + +2. **`StreamDocumentUploadQueue.flushAndPush()` is tightly coupled to `UpdateStreamServiceInternal.createUploadAndPush()`** + - These MUST be in the same PR or the code won't compile + +3. **PlatformClient null check** is a simple fix that could go anywhere + +## Open Questions + +1. Should documentation be bundled with features or kept separate? +2. Should `PlatformClient` null check fix be in PR1 or separate? +3. Is there value in splitting service constructors from the core infrastructure? + +## Proposed Split Strategy (Preliminary) + +### Option A: 3 PRs (Balanced) +- PR1: Core Infrastructure + Service Constructors + Tests +- PR2: File Container Rotation Logic + Tests +- PR3: Documentation + +### Option B: 2 PRs (Simpler) +- PR1: All Code + Tests +- PR2: Documentation + +### Option C: 4 PRs (Maximum Separation) +- PR1: Core Infrastructure (DocumentUploadQueue + PlatformClient fix) +- PR2: Service Constructors (PushService, StreamService, UpdateStreamService) +- PR3: File Container Rotation (StreamDocumentUploadQueue, UpdateStreamServiceInternal) +- PR4: Documentation + +## Research Findings + +- The PR follows Coveo's catalog stream API best practices +- Default batch size increased from 5MB to 256MB (51x increase) +- Breaking behavioral change: each batch now gets its own file container and is pushed immediately +- Project uses Maven, JUnit 4 + Mockito for testing +- Spotless plugin enforces Google Java Format + +## Additional Context from Explore Agent + +### Class Relationships Confirmed +- `DocumentUploadQueue` is the base queue class (package-private) +- `StreamDocumentUploadQueue` extends `DocumentUploadQueue`, adds: + - `PartialUpdateDocument` support + - `flushAndPush()` method that calls `UpdateStreamServiceInternal.createUploadAndPush()` + - `setUpdateStreamService()` to link back to internal service +- `UpdateStreamServiceInternal` orchestrates the 3-step workflow: create container → upload → push +- High-level services (`PushService`, `StreamService`, `UpdateStreamService`) are facades that wire queues + PlatformClient + +### Test Infrastructure +- JUnit 4 with `@Test`, `@Before`, `@After` +- Heavy Mockito usage: `@Mock`, `@InjectMocks`, `verify`, `when`, `ArgumentCaptor` +- Existing batching tests validate system property configuration +- FileContainerRotationIntegrationTest uses mocks for end-to-end container rotation + +### Commit History (6 commits) +1. `45282f1` - feat: add configurable batch size with 256MB default and runtime configuration +2. `8280d47` - Update docs and add UpdateStreamService constructor +3. `68e46e1` - tests: test new file container rotation logic +4. `6d05dfe` - fix issues +5. `66b789e` - fix null user agent +6. `682bb0d` - format + +## Questions for User (Decision Points) +1. Documentation bundling strategy? +2. PlatformClient null check - separate or bundle? +3. Preferred PR count: 2, 3, or 4? diff --git a/.sisyphus/plans/pr-186-split.md b/.sisyphus/plans/pr-186-split.md new file mode 100644 index 00000000..e4e83cbc --- /dev/null +++ b/.sisyphus/plans/pr-186-split.md @@ -0,0 +1,690 @@ +# PR #186 Split Plan: Configurable Batch Size Feature + +## TL;DR + +> **Quick Summary**: Split PR #186 (1368 additions, 136 deletions, 16 files) into 3 smaller, independently mergeable PRs that introduce configurable batch sizing and file container rotation for the Coveo Push API Java Client. +> +> **Deliverables**: +> - PR1: Independent bugfix for null userAgents validation +> - PR2: Batch size configuration infrastructure in DocumentUploadQueue +> - PR3: Service-level integration with configurable batch size + file container rotation + documentation +> +> **Estimated Effort**: Medium (PR extraction from existing branch) +> **Parallel Execution**: NO - sequential dependency chain +> **Critical Path**: PR1 → PR2 → PR3 + +--- + +## Context + +### Original Request +Split PR #186 ("feat: increase batch size to 256MB and add configurable batching with runtime system property") into smaller, independently mergeable PRs. + +### Interview Summary +**Key Decisions**: +- User chose 3-PR split (consolidated from initial 4-PR proposal) +- Documentation bundled with code features +- PlatformClient null check is independent bugfix (PR1) +- UpdateStreamService cannot be split from behavioral changes (tight coupling) + +**Technical Constraints Identified**: +- `StreamDocumentUploadQueue.flushAndPush()` calls `UpdateStreamServiceInternal.createUploadAndPush()` - must be in same PR +- UpdateStreamService's new constructor passes `null` as UploadStrategy - requires behavioral changes to compile +- Each PR must compile and pass tests independently + +### Source Branch +- Branch: `feature/configurable-batch-size-256mb` +- Commits: 6 total (45282f1, 8280d47, 68e46e1, 6d05dfe, 66b789e, 682bb0d) + +--- + +## Work Objectives + +### Core Objective +Extract changes from a single large PR into 3 smaller PRs that can be reviewed and merged independently while maintaining build stability. + +### Concrete Deliverables +- 3 new branches created from `main` +- 3 pull requests with appropriate scope +- Each PR passes `mvn test` independently +- Each PR follows conventional commit format + +### Definition of Done +- [ ] All 3 PRs created and passing CI +- [ ] PR1 merged before PR2, PR2 merged before PR3 +- [ ] Original PR #186 can be closed after PR3 merges +- [ ] `mvn test` passes after each PR merge + +### Must Have +- Each PR compiles independently +- Tests accompany their corresponding code changes +- Clear dependency chain documented in PR descriptions + +### Must NOT Have (Guardrails) +- Do NOT cherry-pick individual commits (they contain mixed changes) +- Do NOT modify logic beyond what's in the original PR +- Do NOT reorder the dependency chain (PR1 → PR2 → PR3) +- Do NOT include documentation in PR1 or PR2 (all docs go in PR3) + +--- + +## Verification Strategy + +### Test Decision +- **Infrastructure exists**: YES (JUnit 4 + Mockito) +- **Test approach**: Tests accompany code changes +- **Framework**: Maven with `mvn test` + +### Verification Commands +```bash +# After each PR's changes are applied: +mvn clean compile # Must succeed +mvn test # Must pass all tests +mvn spotless:check # Must pass formatting +``` + +--- + +## Execution Strategy + +### Dependency Chain (Sequential) + +``` +PR1: fix(PlatformClient): validate userAgents is not null + │ + │ MERGE PR1 + ▼ +PR2: feat(DocumentUploadQueue): add configurable batch size infrastructure + │ + │ MERGE PR2 + ▼ +PR3: feat(services): add configurable batch size with file container rotation +``` + +### Why Sequential (Not Parallel) +- PR2 depends on PR1 for the `userAgents != null` check pattern +- PR3 depends on PR2 for `DocumentUploadQueue.getConfiguredBatchSize()` and `DEFAULT_QUEUE_SIZE` +- Cannot parallelize without creating merge conflicts + +--- + +## TODOs + +--- + +### TODO 1: Create PR1 - PlatformClient Null Check Fix + +- [ ] 1. Create PR1: `fix(PlatformClient): validate userAgents is not null` + +**What to do**: + +1. Create new branch `fix/platform-client-null-userAgents` from `main` +2. Extract ONLY the null check addition from `PlatformClient.java` +3. Create PR with minimal scope + +**Files to include**: + +| File | Change Type | Specific Changes | +|------|-------------|------------------| +| `src/main/java/com/coveo/pushapiclient/PlatformClient.java` | MODIFY | Add null check in `setUserAgents()` method (3 lines) | + +**Exact change to extract** (lines to add at `setUserAgents()` method, before existing validation): +```java +public void setUserAgents(String[] userAgents) { + if (userAgents == null) { + throw new IllegalArgumentException("User agents cannot be null"); + } + // ... existing validation continues +} +``` + +**Must NOT do**: +- Do NOT include any other PlatformClient changes +- Do NOT include any test changes (this is a defensive fix) +- Do NOT include any documentation + +**PR Title**: `fix(PlatformClient): validate userAgents is not null` + +**PR Description**: +```markdown +## Summary +Adds null validation to `PlatformClient.setUserAgents()` to fail fast with a clear error message instead of potentially causing NullPointerException downstream. + +## Changes +- Added null check that throws `IllegalArgumentException` if `userAgents` is null + +## Testing +- Existing tests pass +- This is a defensive fix; callers should not pass null +``` + +**Dependencies**: None (can merge to `main` directly) + +**Recommended Agent Profile**: +- **Category**: `quick` +- **Skills**: [`git-master`] + - `git-master`: Needed for precise git operations (cherry-pick partial changes, create branch) + +**Implementation Approach**: +```bash +# 1. Create branch from main +git checkout main +git pull origin main +git checkout -b fix/platform-client-null-userAgents + +# 2. Extract ONLY the PlatformClient change +# Use git show to view the change, then manually apply just those 3 lines +git show 66b789e:src/main/java/com/coveo/pushapiclient/PlatformClient.java > /tmp/new.java +# Compare and extract only the null check addition + +# 3. Alternative: manual edit +# Add these 3 lines to setUserAgents() method: +# if (userAgents == null) { +# throw new IllegalArgumentException("User agents cannot be null"); +# } + +# 4. Verify +mvn clean compile +mvn test + +# 5. Commit and push +git add src/main/java/com/coveo/pushapiclient/PlatformClient.java +git commit -m "fix(PlatformClient): validate userAgents is not null" +git push -u origin fix/platform-client-null-userAgents + +# 6. Create PR via gh CLI +gh pr create --title "fix(PlatformClient): validate userAgents is not null" \ + --body "## Summary +Adds null validation to \`PlatformClient.setUserAgents()\` to fail fast with a clear error message. + +## Changes +- Added null check that throws \`IllegalArgumentException\` if \`userAgents\` is null + +## Testing +- Existing tests pass" +``` + +**Acceptance Criteria**: +- [ ] `mvn clean compile` succeeds +- [ ] `mvn test` passes (no new test failures) +- [ ] `mvn spotless:check` passes +- [ ] PR contains exactly 1 file changed, +3 lines +- [ ] Calling `setUserAgents(null)` throws `IllegalArgumentException` + +**Commit**: YES +- Message: `fix(PlatformClient): validate userAgents is not null` +- Files: `src/main/java/com/coveo/pushapiclient/PlatformClient.java` +- Pre-commit: `mvn test` + +--- + +### TODO 2: Create PR2 - Batch Size Infrastructure + +- [ ] 2. Create PR2: `feat(DocumentUploadQueue): add configurable batch size infrastructure` + +**What to do**: + +1. Create new branch `feat/configurable-batch-size-infrastructure` from `main` (after PR1 merges) +2. Extract DocumentUploadQueue changes (constants, static method, new constructor) +3. Extract corresponding test updates +4. Ensure backward compatibility (existing constructor still works) + +**Files to include**: + +| File | Change Type | Specific Changes | +|------|-------------|------------------| +| `src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java` | MODIFY | Add constants, `getConfiguredBatchSize()`, new constructor with validation | +| `src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java` | MODIFY | Update tests to use explicit batch size | + +**Exact changes for DocumentUploadQueue.java**: + +1. **Add constants** (after logger declaration): +```java +/** Maximum allowed queue size based on Stream API limit (256 MB) */ +protected static final int MAX_ALLOWED_QUEUE_SIZE = 256 * 1024 * 1024; + +/** Default queue size (256 MB to match API limit) */ +protected static final int DEFAULT_QUEUE_SIZE = 256 * 1024 * 1024; + +/** System property name for configuring the default batch size */ +public static final String BATCH_SIZE_PROPERTY = "coveo.push.batchSize"; +``` + +2. **Add field**: +```java +protected final int maxQueueSize; +``` + +3. **Add static method** `getConfiguredBatchSize()`: +```java +/** + * Gets the configured batch size from system properties, or returns the default if not set. + * + * @return The configured batch size in bytes + * @throws IllegalArgumentException if the configured value exceeds 256MB or is invalid + */ +public static int getConfiguredBatchSize() { + String propertyValue = System.getProperty(BATCH_SIZE_PROPERTY); + if (propertyValue == null || propertyValue.trim().isEmpty()) { + return DEFAULT_QUEUE_SIZE; + } + + try { + int configuredSize = Integer.parseInt(propertyValue.trim()); + if (configuredSize > MAX_ALLOWED_QUEUE_SIZE) { + throw new IllegalArgumentException( + String.format("System property %s (%d bytes) exceeds the Stream API limit of %d bytes (256 MB)", + BATCH_SIZE_PROPERTY, configuredSize, MAX_ALLOWED_QUEUE_SIZE)); + } + if (configuredSize <= 0) { + throw new IllegalArgumentException( + String.format("System property %s must be greater than 0, got: %d", + BATCH_SIZE_PROPERTY, configuredSize)); + } + logger.info(String.format("Using configured batch size from system property %s: %d bytes (%.2f MB)", + BATCH_SIZE_PROPERTY, configuredSize, configuredSize / (1024.0 * 1024.0))); + return configuredSize; + } catch (NumberFormatException e) { + throw new IllegalArgumentException( + String.format("Invalid value for system property %s: '%s'. Must be a valid integer.", + BATCH_SIZE_PROPERTY, propertyValue), e); + } +} +``` + +4. **Modify existing constructor** to delegate: +```java +public DocumentUploadQueue(UploadStrategy uploader) { + this(uploader, getConfiguredBatchSize()); +} +``` + +5. **Add new constructor** with maxQueueSize parameter: +```java +/** + * Constructs a new DocumentUploadQueue object with a configurable maximum queue size limit. + * + * @param uploader The upload strategy to be used for document uploads. + * @param maxQueueSize The maximum queue size in bytes. Must not exceed 256MB (Stream API limit). + * @throws IllegalArgumentException if maxQueueSize exceeds the API limit of 256MB. + */ +public DocumentUploadQueue(UploadStrategy uploader, int maxQueueSize) { + if (maxQueueSize > MAX_ALLOWED_QUEUE_SIZE) { + throw new IllegalArgumentException( + String.format("maxQueueSize (%d bytes) exceeds the Stream API limit of %d bytes (256 MB)", + maxQueueSize, MAX_ALLOWED_QUEUE_SIZE)); + } + if (maxQueueSize <= 0) { + throw new IllegalArgumentException("maxQueueSize must be greater than 0"); + } + this.documentToAddList = new ArrayList<>(); + this.documentToDeleteList = new ArrayList<>(); + this.uploader = uploader; + this.maxQueueSize = maxQueueSize; +} +``` + +6. **Update add() methods** to use `this.maxQueueSize` instead of hardcoded value + +**Exact changes for DocumentUploadQueueTest.java**: +- Update test instantiation to use explicit batch size where needed +- Add tests for `getConfiguredBatchSize()` with system property +- Add tests for constructor validation (exceeds 256MB, negative values) + +**Must NOT do**: +- Do NOT include changes to PushService, StreamService, or UpdateStreamService +- Do NOT include StreamDocumentUploadQueue changes +- Do NOT include any documentation files +- Do NOT change the behavioral contract (flush behavior stays the same) + +**PR Title**: `feat(DocumentUploadQueue): add configurable batch size infrastructure` + +**PR Description**: +```markdown +## Summary +Adds infrastructure for configurable batch sizing in DocumentUploadQueue. This is foundational work that enables services to configure their batch size. + +## Changes +- Added constants: `MAX_ALLOWED_QUEUE_SIZE` (256MB), `DEFAULT_QUEUE_SIZE` (256MB), `BATCH_SIZE_PROPERTY` +- Added `getConfiguredBatchSize()` static method for system property configuration +- Added new constructor with `maxQueueSize` parameter +- Added validation to prevent exceeding 256MB API limit +- Updated existing constructor to use `getConfiguredBatchSize()` as default + +## Configuration +- System property: `-Dcoveo.push.batchSize=` +- Constructor parameter: `new DocumentUploadQueue(uploader, maxQueueSize)` +- Priority: constructor > system property > default (256MB) + +## Testing +- Updated existing tests +- Added tests for system property configuration +- Added tests for validation (max size, invalid values) +``` + +**Dependencies**: PR1 must be merged first (for consistent null handling patterns) + +**Recommended Agent Profile**: +- **Category**: `visual-engineering` +- **Skills**: [`git-master`] + - `git-master`: Needed for precise extraction of changes from existing branch + +**Implementation Approach**: +```bash +# 1. Ensure PR1 is merged, then create branch from main +git checkout main +git pull origin main +git checkout -b feat/configurable-batch-size-infrastructure + +# 2. Extract DocumentUploadQueue changes from feature branch +# Option A: Use git show to get the file and manually extract relevant parts +git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java > /tmp/new-queue.java +# Then carefully copy only the infrastructure changes (not behavioral changes) + +# Option B: Apply changes manually based on the spec above + +# 3. Extract test changes +git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java > /tmp/new-test.java +# Copy relevant test updates + +# 4. Verify +mvn clean compile +mvn test +mvn spotless:check + +# 5. Format code +mvn spotless:apply + +# 6. Commit and push +git add src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java +git add src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java +git commit -m "feat(DocumentUploadQueue): add configurable batch size infrastructure" +git push -u origin feat/configurable-batch-size-infrastructure + +# 7. Create PR +gh pr create --title "feat(DocumentUploadQueue): add configurable batch size infrastructure" --body "..." +``` + +**Acceptance Criteria**: +- [ ] `mvn clean compile` succeeds +- [ ] `mvn test` passes +- [ ] `mvn spotless:check` passes +- [ ] `DocumentUploadQueue.getConfiguredBatchSize()` returns 256MB by default +- [ ] System property `coveo.push.batchSize` overrides default +- [ ] Constructor with invalid maxQueueSize throws `IllegalArgumentException` +- [ ] Existing code using `new DocumentUploadQueue(uploader)` still works + +**Verification Script**: +```bash +# Test system property configuration +mvn test -Dtest=DocumentUploadQueueTest -Dcoveo.push.batchSize=134217728 +``` + +**Commit**: YES +- Message: `feat(DocumentUploadQueue): add configurable batch size infrastructure` +- Files: `DocumentUploadQueue.java`, `DocumentUploadQueueTest.java` +- Pre-commit: `mvn test` + +--- + +### TODO 3: Create PR3 - Services + File Container Rotation + Documentation + +- [ ] 3. Create PR3: `feat(services): add configurable batch size with file container rotation` + +**What to do**: + +1. Create new branch `feat/configurable-batch-size-services` from `main` (after PR2 merges) +2. Extract ALL remaining changes from the feature branch +3. This is the largest PR - contains service updates, behavioral changes, tests, and documentation + +**Files to include**: + +| File | Change Type | Specific Changes | +|------|-------------|------------------| +| `src/main/java/com/coveo/pushapiclient/PushService.java` | MODIFY | New constructor with `maxQueueSize` | +| `src/main/java/com/coveo/pushapiclient/StreamService.java` | MODIFY | New constructors with `maxQueueSize`, update existing constructors | +| `src/main/java/com/coveo/pushapiclient/UpdateStreamService.java` | MODIFY | New constructors, remove `fileContainer` field, remove `getUploadStrategy()`, update method bodies | +| `src/main/java/com/coveo/pushapiclient/StreamDocumentUploadQueue.java` | MODIFY | New constructor, `setUpdateStreamService()`, `flushAndPush()`, override `add()` methods | +| `src/main/java/com/coveo/pushapiclient/UpdateStreamServiceInternal.java` | MODIFY | Remove file container creation from `add*()`, add `createUploadAndPush()`, modify `close()` | +| `src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueTest.java` | MODIFY | Update tests for new behavior | +| `src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueBatchingTest.java` | NEW | New test file (274 lines) | +| `src/test/java/com/coveo/pushapiclient/FileContainerRotationIntegrationTest.java` | NEW | New test file (225 lines) | +| `src/test/java/com/coveo/pushapiclient/UpdateStreamServiceInternalTest.java` | MODIFY | Update tests for new behavior | +| `CONFIGURATION.md` | NEW | Configuration guide (235 lines) | +| `UPGRADE_NOTES.md` | NEW | Migration guidance (171 lines) | +| `README.md` | MODIFY | Add configuration section (24 lines) | +| `samples/UpdateStreamDocuments.java` | MODIFY | Add explanatory comments (8 lines) | + +**Detailed changes by file**: + +#### PushService.java +- Add new constructor: `PushService(source, options, int maxQueueSize)` +- Update existing constructors to use `DEFAULT_QUEUE_SIZE` +- Pass `maxQueueSize` to `DocumentUploadQueue` constructor + +#### StreamService.java +- Add new 4-param constructor: `StreamService(source, options, userAgents, int maxQueueSize)` +- Update all existing constructors to delegate to the 4-param version +- Add `userAgents != null` check before calling `setUserAgents()` +- Pass `maxQueueSize` to `DocumentUploadQueue` constructor + +#### UpdateStreamService.java +- Remove `private FileContainer fileContainer;` field +- Add new 4-param constructor with `maxQueueSize` +- Update all existing constructors to delegate to 4-param version +- Add `userAgents != null` check +- Pass `null` for UploadStrategy and `maxQueueSize` to `StreamDocumentUploadQueue` +- Remove `getUploadStrategy()` method +- Update `addOrUpdate()`, `addPartialUpdate()`, `delete()` to not capture return value +- Update Javadoc to reflect new behavior + +#### StreamDocumentUploadQueue.java +- Add field: `private UpdateStreamServiceInternal updateStreamService;` +- Add new constructor: `StreamDocumentUploadQueue(uploader, int maxQueueSize)` +- Add method: `setUpdateStreamService(UpdateStreamServiceInternal)` +- Add method: `flushAndPush()` - creates container, uploads, pushes +- Override `flush()` to check for null uploader +- Override `add(DocumentBuilder)` to call `flushAndPush()` on batch limit +- Override `add(DeleteDocument)` to call `flushAndPush()` on batch limit +- Override `add(PartialUpdateDocument)` to call `flushAndPush()` on batch limit + +#### UpdateStreamServiceInternal.java +- Add `queue.setUpdateStreamService(this);` in constructor +- Remove `if (this.fileContainer == null)` checks from `addOrUpdate()`, `addPartialUpdate()`, `delete()` +- Add new method: `createUploadAndPush(StreamUpdate)` - handles create→upload→push workflow +- Modify `close()` to call `queue.flushAndPush()` instead of `pushFileContainer()` +- Remove `createFileContainer()` private method +- Remove `pushFileContainer()` private method + +**Must NOT do**: +- Do NOT modify DocumentUploadQueue.java (already done in PR2) +- Do NOT modify PlatformClient.java (already done in PR1) +- Do NOT change any logic that wasn't in the original PR + +**PR Title**: `feat(services): add configurable batch size with file container rotation` + +**PR Description**: +```markdown +## Summary +Adds configurable batch size support to all push services and implements per-batch file container rotation for UpdateStreamService following Coveo's catalog stream API best practices. + +## Key Changes + +### Configurable Batch Size +All services now support custom batch sizes (max: 256MB): +- `PushService(source, options, maxQueueSize)` +- `StreamService(source, options, userAgents, maxQueueSize)` +- `UpdateStreamService(source, options, userAgents, maxQueueSize)` + +### File Container Rotation (UpdateStreamService) +Each batch now gets its own file container: +1. Create new file container +2. Upload batch content +3. Push container immediately + +This follows the [catalog stream API best practices](https://docs.coveo.com/en/p4eb0129/coveo-for-commerce/full-catalog-data-updates#update-operations). + +## Breaking Changes +- Default batch size increased from 5MB to 256MB +- UpdateStreamService pushes each batch immediately instead of accumulating + +## Documentation +- Added CONFIGURATION.md with complete configuration guide +- Added UPGRADE_NOTES.md with migration guidance +- Updated README.md with configuration section + +## Testing +- Added StreamDocumentUploadQueueBatchingTest (new) +- Added FileContainerRotationIntegrationTest (new) +- Updated existing tests for new behavior +``` + +**Dependencies**: PR2 must be merged first + +**Recommended Agent Profile**: +- **Category**: `visual-engineering` +- **Skills**: [`git-master`] + - `git-master`: Complex git operations to extract remaining changes + +**Implementation Approach**: +```bash +# 1. Ensure PR2 is merged, then create branch from main +git checkout main +git pull origin main +git checkout -b feat/configurable-batch-size-services + +# 2. Since PR1 and PR2 changes are now in main, we can cherry-pick +# the remaining changes more easily. However, commits are mixed. +# Best approach: copy files from feature branch and let git diff handle it + +# For each source file: +git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/PushService.java > src/main/java/com/coveo/pushapiclient/PushService.java +git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/StreamService.java > src/main/java/com/coveo/pushapiclient/StreamService.java +git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/UpdateStreamService.java > src/main/java/com/coveo/pushapiclient/UpdateStreamService.java +git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/StreamDocumentUploadQueue.java > src/main/java/com/coveo/pushapiclient/StreamDocumentUploadQueue.java +git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/UpdateStreamServiceInternal.java > src/main/java/com/coveo/pushapiclient/UpdateStreamServiceInternal.java + +# For each test file: +git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueTest.java > src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueTest.java +git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueBatchingTest.java > src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueBatchingTest.java +git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/FileContainerRotationIntegrationTest.java > src/test/java/com/coveo/pushapiclient/FileContainerRotationIntegrationTest.java +git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/UpdateStreamServiceInternalTest.java > src/test/java/com/coveo/pushapiclient/UpdateStreamServiceInternalTest.java + +# For documentation: +git show feature/configurable-batch-size-256mb:CONFIGURATION.md > CONFIGURATION.md +git show feature/configurable-batch-size-256mb:UPGRADE_NOTES.md > UPGRADE_NOTES.md +git show feature/configurable-batch-size-256mb:README.md > README.md +git show feature/configurable-batch-size-256mb:samples/UpdateStreamDocuments.java > samples/UpdateStreamDocuments.java + +# 3. Verify +mvn clean compile +mvn test +mvn spotless:check + +# 4. Format if needed +mvn spotless:apply + +# 5. Stage all changes +git add -A + +# 6. Commit +git commit -m "feat(services): add configurable batch size with file container rotation + +- Add configurable batch size to PushService, StreamService, UpdateStreamService +- Implement per-batch file container rotation for UpdateStreamService +- Add CONFIGURATION.md and UPGRADE_NOTES.md documentation +- Update README.md with configuration section +- Add comprehensive tests for batching and rotation behavior" + +# 7. Push and create PR +git push -u origin feat/configurable-batch-size-services +gh pr create --title "feat(services): add configurable batch size with file container rotation" --body "..." +``` + +**Acceptance Criteria**: + +**Compilation & Tests**: +- [ ] `mvn clean compile` succeeds +- [ ] `mvn test` passes all tests (including new tests) +- [ ] `mvn spotless:check` passes + +**PushService**: +- [ ] New constructor `PushService(source, options, maxQueueSize)` available +- [ ] Default constructor uses 256MB batch size +- [ ] Batching works correctly with custom size + +**StreamService**: +- [ ] New constructor `StreamService(source, options, userAgents, maxQueueSize)` available +- [ ] All existing constructors still work +- [ ] `userAgents` can be null without error + +**UpdateStreamService**: +- [ ] New constructor `UpdateStreamService(source, options, userAgents, maxQueueSize)` available +- [ ] Each batch creates its own file container (verify via logs) +- [ ] Batches are pushed immediately when limit exceeded +- [ ] `close()` pushes remaining documents + +**Documentation**: +- [ ] CONFIGURATION.md exists with complete guide +- [ ] UPGRADE_NOTES.md explains behavioral changes +- [ ] README.md has new Configuration section + +**Commit**: YES +- Message: `feat(services): add configurable batch size with file container rotation` +- Files: 13 files (5 source, 4 test, 4 doc) +- Pre-commit: `mvn test` + +--- + +## Commit Strategy + +| PR | After Merge | Commit Message | Key Files | +|----|-------------|----------------|-----------| +| 1 | PR1 | `fix(PlatformClient): validate userAgents is not null` | PlatformClient.java | +| 2 | PR2 | `feat(DocumentUploadQueue): add configurable batch size infrastructure` | DocumentUploadQueue.java, DocumentUploadQueueTest.java | +| 3 | PR3 | `feat(services): add configurable batch size with file container rotation` | 13 files | + +--- + +## Success Criteria + +### Verification Commands +```bash +# After all PRs merged: +git checkout main +git pull origin main +mvn clean test # Expected: BUILD SUCCESS +mvn spotless:check # Expected: no violations + +# Verify batch size configuration +mvn test -Dcoveo.push.batchSize=134217728 # Should use 128MB +``` + +### Final Checklist +- [ ] PR1 merged and passing CI +- [ ] PR2 merged and passing CI +- [ ] PR3 merged and passing CI +- [ ] Original PR #186 closed (superseded) +- [ ] All 16 files from original PR are accounted for +- [ ] Documentation is complete and accurate +- [ ] Behavioral change (file container rotation) is working + +--- + +## Risks and Mitigations + +| Risk | Likelihood | Impact | Mitigation | +|------|------------|--------|------------| +| Merge conflicts between PRs | Low | Medium | Merge PRs in strict order; don't parallelize | +| PR2 breaks existing code | Low | High | Run full test suite; existing constructor unchanged | +| PR3 tests fail due to missing PR2 infrastructure | Medium | Medium | Ensure PR2 is fully merged before creating PR3 branch | +| Documentation out of sync | Low | Low | Include all docs in PR3 to keep them together | + +--- + +## Post-Merge Cleanup + +After PR3 is merged: +1. Close original PR #186 with comment: "Superseded by #PR1, #PR2, #PR3" +2. Delete feature branch: `git push origin --delete feature/configurable-batch-size-256mb` +3. Delete local branches created for split PRs (optional) diff --git a/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java b/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java index d3cc8480..87896c64 100644 --- a/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java +++ b/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java @@ -8,21 +8,112 @@ /** Represents a queue for uploading documents using a specified upload strategy */ class DocumentUploadQueue { private static final Logger logger = LogManager.getLogger(DocumentUploadQueue.class); - protected final UploadStrategy uploader; - protected final int maxQueueSize = 5 * 1024 * 1024; + + /** Maximum allowed queue size based on Stream API limit (256 MB) */ + protected static final int MAX_ALLOWED_QUEUE_SIZE = 256 * 1024 * 1024; + + /** Default queue size (5 MB) */ + protected static final int DEFAULT_QUEUE_SIZE = 5 * 1024 * 1024; + + /** System property name for configuring the default batch size */ + public static final String BATCH_SIZE_PROPERTY = "coveo.push.batchSize"; + + protected UploadStrategy uploader; + protected final int maxQueueSize; protected ArrayList documentToAddList; protected ArrayList documentToDeleteList; protected int size; + + /** + * Validates batch size against constraints (> 0 and <= 256MB). + * Used by getConfiguredBatchSize and constructors to ensure consistent validation logic. + * + * @param sizeBytes The batch size in bytes to validate + * @throws IllegalArgumentException if size exceeds MAX_ALLOWED_QUEUE_SIZE or is <= 0 + */ + protected static void validateBatchSize(int sizeBytes) { + if (sizeBytes > MAX_ALLOWED_QUEUE_SIZE) { + throw new IllegalArgumentException( + String.format( + "Batch size (%d bytes) exceeds the Stream API limit of %d bytes (%d MB)", + sizeBytes, MAX_ALLOWED_QUEUE_SIZE, MAX_ALLOWED_QUEUE_SIZE / (1024 * 1024))); + } + if (sizeBytes <= 0) { + throw new IllegalArgumentException("Batch size must be greater than 0"); + } + } + + /** + * Gets the configured batch size from system properties, or returns the default if not set. + * + * The system property is read as bytes. When not set, returns DEFAULT_QUEUE_SIZE (5 MB). + * + * Example: Set a 50 MB batch size via system property: + * 
+   *   java -Dcoveo.push.batchSize=52428800 -jar app.jar  // 50 * 1024 * 1024 bytes
+   *
+ * + * @return The configured batch size in bytes (e.g., 52428800 for 50 MB) + * @throws IllegalArgumentException if the configured value exceeds 256MB or is invalid + */ + public static int getConfiguredBatchSize() { + String propertyValue = System.getProperty(BATCH_SIZE_PROPERTY); + if (propertyValue == null || propertyValue.trim().isEmpty()) { + return DEFAULT_QUEUE_SIZE; + } + + int configuredSize; + try { + configuredSize = Integer.parseInt(propertyValue.trim()); + } catch (NumberFormatException e) { + throw new IllegalArgumentException( + String.format("Invalid value for system property %s: '%s'. Must be a valid integer.", + BATCH_SIZE_PROPERTY, propertyValue), e); + } + + validateBatchSize(configuredSize); + + logger.info(String.format("Using configured batch size from system property %s: %d bytes (%.2f MB)", + BATCH_SIZE_PROPERTY, configuredSize, configuredSize / (1024.0 * 1024.0))); + return configuredSize; + } /** - * Constructs a new DocumentUploadQueue object with a default maximum queue size limit of 5MB. + * Constructs a new DocumentUploadQueue with the default batch size. + * + * Uses the configured batch size from system property "coveo.push.batchSize" if set, + * otherwise defaults to DEFAULT_QUEUE_SIZE (5 MB = 5242880 bytes). * * @param uploader The upload strategy to be used for document uploads. + * @throws IllegalArgumentException if the system property value exceeds 256MB or is invalid. */ public DocumentUploadQueue(UploadStrategy uploader) { + this(uploader, getConfiguredBatchSize()); + } + + /** + * Constructs a new DocumentUploadQueue object with a configurable maximum queue size limit. + * + * @param uploader The upload strategy to be used for document uploads. + * @param maxQueueSize The maximum queue size in bytes (e.g., 52428800 for 50 MB). Must not exceed 256MB (Stream API limit). + * @throws IllegalArgumentException if maxQueueSize exceeds the API limit of 256MB. + */ + public DocumentUploadQueue(UploadStrategy uploader, int maxQueueSize) { + validateBatchSize(maxQueueSize); this.documentToAddList = new ArrayList<>(); this.documentToDeleteList = new ArrayList<>(); this.uploader = uploader; + this.maxQueueSize = maxQueueSize; + } + + /** + * Default constructor for testing purposes (used by Mockito @InjectMocks). + * Initializes with default batch size; uploader is injected by Mockito. + */ + public DocumentUploadQueue() { + this.documentToAddList = new ArrayList<>(); + this.documentToDeleteList = new ArrayList<>(); + this.maxQueueSize = DEFAULT_QUEUE_SIZE; } /** diff --git a/src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java b/src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java index f488f3a5..51a3541e 100644 --- a/src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java +++ b/src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java @@ -12,16 +12,16 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; -import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.MockitoAnnotations; public class DocumentUploadQueueTest { - @Mock private UploadStrategy uploadStrategy; + private static final int TEST_BATCH_SIZE = 5 * 1024 * 1024; - @InjectMocks private DocumentUploadQueue queue; + @Mock private UploadStrategy uploadStrategy; + private DocumentUploadQueue queue; private AutoCloseable closeable; private DocumentBuilder documentToAdd; private DeleteDocument documentToDelete; @@ -29,20 +29,14 @@ public class DocumentUploadQueueTest { private int oneMegaByte = 1 * 1024 * 1024; private String generateStringFromBytes(int numBytes) { - // Check if the number of bytes is valid if (numBytes <= 0) { return ""; } - - // Create a byte array with the specified length byte[] bytes = new byte[numBytes]; - - // Fill the byte array with a pattern of ASCII characters - byte pattern = 65; // ASCII value for 'A' + byte pattern = 65; for (int i = 0; i < numBytes; i++) { bytes[i] = pattern; } - return new String(bytes); } @@ -53,6 +47,10 @@ private DocumentBuilder generateDocumentFromSize(int numBytes) { @Before public void setup() { + closeable = MockitoAnnotations.openMocks(this); + + queue = new DocumentUploadQueue(uploadStrategy, TEST_BATCH_SIZE); + String twoMegaByteData = generateStringFromBytes(2 * oneMegaByte); documentToAdd = @@ -60,8 +58,6 @@ public void setup() { .withData(twoMegaByteData); documentToDelete = new DeleteDocument("https://my.document.uri?ref=3"); - - closeable = MockitoAnnotations.openMocks(this); } @After From 984ce5ca60ac863378d11520b69b122fb10e3196 Mon Sep 17 00:00:00 2001 From: ylakhdar Date: Thu, 29 Jan 2026 11:26:23 -0500 Subject: [PATCH 3/5] remove sisyphus --- .sisyphus/drafts/pr-split-186.md | 124 ------ .sisyphus/plans/pr-186-split.md | 690 ------------------------------- 2 files changed, 814 deletions(-) delete mode 100644 .sisyphus/drafts/pr-split-186.md delete mode 100644 .sisyphus/plans/pr-186-split.md diff --git a/.sisyphus/drafts/pr-split-186.md b/.sisyphus/drafts/pr-split-186.md deleted file mode 100644 index e59ec249..00000000 --- a/.sisyphus/drafts/pr-split-186.md +++ /dev/null @@ -1,124 +0,0 @@ -# Draft: PR #186 Split Strategy - -## User's Request -Split PR #186 (feat: increase batch size to 256MB and add configurable batching) into 2-4 smaller, independently mergeable PRs. - -## PR Analysis Summary - -**Total Changes**: 1368 additions, 136 deletions, 16 files -**Branch**: `feature/configurable-batch-size-256mb` (6 commits) - -### Change Categories Identified - -1. **Core Infrastructure** (foundational - everything depends on this) - - `DocumentUploadQueue.java` - Constants, `getConfiguredBatchSize()`, new constructor with maxQueueSize - - `PlatformClient.java` - Null check for `setUserAgents()` (minor fix) - -2. **Service Layer Updates** (depends on #1) - - `PushService.java` - New constructor with maxQueueSize - - `StreamService.java` - New constructor with maxQueueSize - - `UpdateStreamService.java` - New constructors with maxQueueSize - -3. **Behavioral Change: File Container Rotation** (depends on #1, #2) - - `StreamDocumentUploadQueue.java` - `setUpdateStreamService()`, `flushAndPush()`, overridden `add()` methods - - `UpdateStreamServiceInternal.java` - `createUploadAndPush()`, modified `close()` - -4. **Documentation** - - `CONFIGURATION.md` (new, 235 lines) - - `UPGRADE_NOTES.md` (new, 171 lines) - - `README.md` (24 lines added) - - `samples/UpdateStreamDocuments.java` (8 lines) - -5. **Tests** - - `DocumentUploadQueueTest.java` - Updated for explicit batch size - - `StreamDocumentUploadQueueTest.java` - Updated tests - - `StreamDocumentUploadQueueBatchingTest.java` (new, 274 lines) - - `FileContainerRotationIntegrationTest.java` (new, 225 lines) - - `UpdateStreamServiceInternalTest.java` - Updated tests - -## Key Dependency Analysis - -``` -DocumentUploadQueue (constants + getConfiguredBatchSize + constructor) - │ - ├──► PushService (uses DEFAULT_QUEUE_SIZE, new constructor) - ├──► StreamService (uses getConfiguredBatchSize(), new constructor) - └──► UpdateStreamService (uses getConfiguredBatchSize(), new constructor) - │ - └──► StreamDocumentUploadQueue (extends DocumentUploadQueue) - │ - └──► UpdateStreamServiceInternal (uses queue.flushAndPush()) -``` - -## Technical Constraints - -1. **`DocumentUploadQueue` changes are foundational** - Services reference: - - `DEFAULT_QUEUE_SIZE` (constant) - - `getConfiguredBatchSize()` (static method) - - Constructor with `maxQueueSize` parameter - -2. **`StreamDocumentUploadQueue.flushAndPush()` is tightly coupled to `UpdateStreamServiceInternal.createUploadAndPush()`** - - These MUST be in the same PR or the code won't compile - -3. **PlatformClient null check** is a simple fix that could go anywhere - -## Open Questions - -1. Should documentation be bundled with features or kept separate? -2. Should `PlatformClient` null check fix be in PR1 or separate? -3. Is there value in splitting service constructors from the core infrastructure? - -## Proposed Split Strategy (Preliminary) - -### Option A: 3 PRs (Balanced) -- PR1: Core Infrastructure + Service Constructors + Tests -- PR2: File Container Rotation Logic + Tests -- PR3: Documentation - -### Option B: 2 PRs (Simpler) -- PR1: All Code + Tests -- PR2: Documentation - -### Option C: 4 PRs (Maximum Separation) -- PR1: Core Infrastructure (DocumentUploadQueue + PlatformClient fix) -- PR2: Service Constructors (PushService, StreamService, UpdateStreamService) -- PR3: File Container Rotation (StreamDocumentUploadQueue, UpdateStreamServiceInternal) -- PR4: Documentation - -## Research Findings - -- The PR follows Coveo's catalog stream API best practices -- Default batch size increased from 5MB to 256MB (51x increase) -- Breaking behavioral change: each batch now gets its own file container and is pushed immediately -- Project uses Maven, JUnit 4 + Mockito for testing -- Spotless plugin enforces Google Java Format - -## Additional Context from Explore Agent - -### Class Relationships Confirmed -- `DocumentUploadQueue` is the base queue class (package-private) -- `StreamDocumentUploadQueue` extends `DocumentUploadQueue`, adds: - - `PartialUpdateDocument` support - - `flushAndPush()` method that calls `UpdateStreamServiceInternal.createUploadAndPush()` - - `setUpdateStreamService()` to link back to internal service -- `UpdateStreamServiceInternal` orchestrates the 3-step workflow: create container → upload → push -- High-level services (`PushService`, `StreamService`, `UpdateStreamService`) are facades that wire queues + PlatformClient - -### Test Infrastructure -- JUnit 4 with `@Test`, `@Before`, `@After` -- Heavy Mockito usage: `@Mock`, `@InjectMocks`, `verify`, `when`, `ArgumentCaptor` -- Existing batching tests validate system property configuration -- FileContainerRotationIntegrationTest uses mocks for end-to-end container rotation - -### Commit History (6 commits) -1. `45282f1` - feat: add configurable batch size with 256MB default and runtime configuration -2. `8280d47` - Update docs and add UpdateStreamService constructor -3. `68e46e1` - tests: test new file container rotation logic -4. `6d05dfe` - fix issues -5. `66b789e` - fix null user agent -6. `682bb0d` - format - -## Questions for User (Decision Points) -1. Documentation bundling strategy? -2. PlatformClient null check - separate or bundle? -3. Preferred PR count: 2, 3, or 4? diff --git a/.sisyphus/plans/pr-186-split.md b/.sisyphus/plans/pr-186-split.md deleted file mode 100644 index e4e83cbc..00000000 --- a/.sisyphus/plans/pr-186-split.md +++ /dev/null @@ -1,690 +0,0 @@ -# PR #186 Split Plan: Configurable Batch Size Feature - -## TL;DR - -> **Quick Summary**: Split PR #186 (1368 additions, 136 deletions, 16 files) into 3 smaller, independently mergeable PRs that introduce configurable batch sizing and file container rotation for the Coveo Push API Java Client. -> -> **Deliverables**: -> - PR1: Independent bugfix for null userAgents validation -> - PR2: Batch size configuration infrastructure in DocumentUploadQueue -> - PR3: Service-level integration with configurable batch size + file container rotation + documentation -> -> **Estimated Effort**: Medium (PR extraction from existing branch) -> **Parallel Execution**: NO - sequential dependency chain -> **Critical Path**: PR1 → PR2 → PR3 - ---- - -## Context - -### Original Request -Split PR #186 ("feat: increase batch size to 256MB and add configurable batching with runtime system property") into smaller, independently mergeable PRs. - -### Interview Summary -**Key Decisions**: -- User chose 3-PR split (consolidated from initial 4-PR proposal) -- Documentation bundled with code features -- PlatformClient null check is independent bugfix (PR1) -- UpdateStreamService cannot be split from behavioral changes (tight coupling) - -**Technical Constraints Identified**: -- `StreamDocumentUploadQueue.flushAndPush()` calls `UpdateStreamServiceInternal.createUploadAndPush()` - must be in same PR -- UpdateStreamService's new constructor passes `null` as UploadStrategy - requires behavioral changes to compile -- Each PR must compile and pass tests independently - -### Source Branch -- Branch: `feature/configurable-batch-size-256mb` -- Commits: 6 total (45282f1, 8280d47, 68e46e1, 6d05dfe, 66b789e, 682bb0d) - ---- - -## Work Objectives - -### Core Objective -Extract changes from a single large PR into 3 smaller PRs that can be reviewed and merged independently while maintaining build stability. - -### Concrete Deliverables -- 3 new branches created from `main` -- 3 pull requests with appropriate scope -- Each PR passes `mvn test` independently -- Each PR follows conventional commit format - -### Definition of Done -- [ ] All 3 PRs created and passing CI -- [ ] PR1 merged before PR2, PR2 merged before PR3 -- [ ] Original PR #186 can be closed after PR3 merges -- [ ] `mvn test` passes after each PR merge - -### Must Have -- Each PR compiles independently -- Tests accompany their corresponding code changes -- Clear dependency chain documented in PR descriptions - -### Must NOT Have (Guardrails) -- Do NOT cherry-pick individual commits (they contain mixed changes) -- Do NOT modify logic beyond what's in the original PR -- Do NOT reorder the dependency chain (PR1 → PR2 → PR3) -- Do NOT include documentation in PR1 or PR2 (all docs go in PR3) - ---- - -## Verification Strategy - -### Test Decision -- **Infrastructure exists**: YES (JUnit 4 + Mockito) -- **Test approach**: Tests accompany code changes -- **Framework**: Maven with `mvn test` - -### Verification Commands -```bash -# After each PR's changes are applied: -mvn clean compile # Must succeed -mvn test # Must pass all tests -mvn spotless:check # Must pass formatting -``` - ---- - -## Execution Strategy - -### Dependency Chain (Sequential) - -``` -PR1: fix(PlatformClient): validate userAgents is not null - │ - │ MERGE PR1 - ▼ -PR2: feat(DocumentUploadQueue): add configurable batch size infrastructure - │ - │ MERGE PR2 - ▼ -PR3: feat(services): add configurable batch size with file container rotation -``` - -### Why Sequential (Not Parallel) -- PR2 depends on PR1 for the `userAgents != null` check pattern -- PR3 depends on PR2 for `DocumentUploadQueue.getConfiguredBatchSize()` and `DEFAULT_QUEUE_SIZE` -- Cannot parallelize without creating merge conflicts - ---- - -## TODOs - ---- - -### TODO 1: Create PR1 - PlatformClient Null Check Fix - -- [ ] 1. Create PR1: `fix(PlatformClient): validate userAgents is not null` - -**What to do**: - -1. Create new branch `fix/platform-client-null-userAgents` from `main` -2. Extract ONLY the null check addition from `PlatformClient.java` -3. Create PR with minimal scope - -**Files to include**: - -| File | Change Type | Specific Changes | -|------|-------------|------------------| -| `src/main/java/com/coveo/pushapiclient/PlatformClient.java` | MODIFY | Add null check in `setUserAgents()` method (3 lines) | - -**Exact change to extract** (lines to add at `setUserAgents()` method, before existing validation): -```java -public void setUserAgents(String[] userAgents) { - if (userAgents == null) { - throw new IllegalArgumentException("User agents cannot be null"); - } - // ... existing validation continues -} -``` - -**Must NOT do**: -- Do NOT include any other PlatformClient changes -- Do NOT include any test changes (this is a defensive fix) -- Do NOT include any documentation - -**PR Title**: `fix(PlatformClient): validate userAgents is not null` - -**PR Description**: -```markdown -## Summary -Adds null validation to `PlatformClient.setUserAgents()` to fail fast with a clear error message instead of potentially causing NullPointerException downstream. - -## Changes -- Added null check that throws `IllegalArgumentException` if `userAgents` is null - -## Testing -- Existing tests pass -- This is a defensive fix; callers should not pass null -``` - -**Dependencies**: None (can merge to `main` directly) - -**Recommended Agent Profile**: -- **Category**: `quick` -- **Skills**: [`git-master`] - - `git-master`: Needed for precise git operations (cherry-pick partial changes, create branch) - -**Implementation Approach**: -```bash -# 1. Create branch from main -git checkout main -git pull origin main -git checkout -b fix/platform-client-null-userAgents - -# 2. Extract ONLY the PlatformClient change -# Use git show to view the change, then manually apply just those 3 lines -git show 66b789e:src/main/java/com/coveo/pushapiclient/PlatformClient.java > /tmp/new.java -# Compare and extract only the null check addition - -# 3. Alternative: manual edit -# Add these 3 lines to setUserAgents() method: -# if (userAgents == null) { -# throw new IllegalArgumentException("User agents cannot be null"); -# } - -# 4. Verify -mvn clean compile -mvn test - -# 5. Commit and push -git add src/main/java/com/coveo/pushapiclient/PlatformClient.java -git commit -m "fix(PlatformClient): validate userAgents is not null" -git push -u origin fix/platform-client-null-userAgents - -# 6. Create PR via gh CLI -gh pr create --title "fix(PlatformClient): validate userAgents is not null" \ - --body "## Summary -Adds null validation to \`PlatformClient.setUserAgents()\` to fail fast with a clear error message. - -## Changes -- Added null check that throws \`IllegalArgumentException\` if \`userAgents\` is null - -## Testing -- Existing tests pass" -``` - -**Acceptance Criteria**: -- [ ] `mvn clean compile` succeeds -- [ ] `mvn test` passes (no new test failures) -- [ ] `mvn spotless:check` passes -- [ ] PR contains exactly 1 file changed, +3 lines -- [ ] Calling `setUserAgents(null)` throws `IllegalArgumentException` - -**Commit**: YES -- Message: `fix(PlatformClient): validate userAgents is not null` -- Files: `src/main/java/com/coveo/pushapiclient/PlatformClient.java` -- Pre-commit: `mvn test` - ---- - -### TODO 2: Create PR2 - Batch Size Infrastructure - -- [ ] 2. Create PR2: `feat(DocumentUploadQueue): add configurable batch size infrastructure` - -**What to do**: - -1. Create new branch `feat/configurable-batch-size-infrastructure` from `main` (after PR1 merges) -2. Extract DocumentUploadQueue changes (constants, static method, new constructor) -3. Extract corresponding test updates -4. Ensure backward compatibility (existing constructor still works) - -**Files to include**: - -| File | Change Type | Specific Changes | -|------|-------------|------------------| -| `src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java` | MODIFY | Add constants, `getConfiguredBatchSize()`, new constructor with validation | -| `src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java` | MODIFY | Update tests to use explicit batch size | - -**Exact changes for DocumentUploadQueue.java**: - -1. **Add constants** (after logger declaration): -```java -/** Maximum allowed queue size based on Stream API limit (256 MB) */ -protected static final int MAX_ALLOWED_QUEUE_SIZE = 256 * 1024 * 1024; - -/** Default queue size (256 MB to match API limit) */ -protected static final int DEFAULT_QUEUE_SIZE = 256 * 1024 * 1024; - -/** System property name for configuring the default batch size */ -public static final String BATCH_SIZE_PROPERTY = "coveo.push.batchSize"; -``` - -2. **Add field**: -```java -protected final int maxQueueSize; -``` - -3. **Add static method** `getConfiguredBatchSize()`: -```java -/** - * Gets the configured batch size from system properties, or returns the default if not set. - * - * @return The configured batch size in bytes - * @throws IllegalArgumentException if the configured value exceeds 256MB or is invalid - */ -public static int getConfiguredBatchSize() { - String propertyValue = System.getProperty(BATCH_SIZE_PROPERTY); - if (propertyValue == null || propertyValue.trim().isEmpty()) { - return DEFAULT_QUEUE_SIZE; - } - - try { - int configuredSize = Integer.parseInt(propertyValue.trim()); - if (configuredSize > MAX_ALLOWED_QUEUE_SIZE) { - throw new IllegalArgumentException( - String.format("System property %s (%d bytes) exceeds the Stream API limit of %d bytes (256 MB)", - BATCH_SIZE_PROPERTY, configuredSize, MAX_ALLOWED_QUEUE_SIZE)); - } - if (configuredSize <= 0) { - throw new IllegalArgumentException( - String.format("System property %s must be greater than 0, got: %d", - BATCH_SIZE_PROPERTY, configuredSize)); - } - logger.info(String.format("Using configured batch size from system property %s: %d bytes (%.2f MB)", - BATCH_SIZE_PROPERTY, configuredSize, configuredSize / (1024.0 * 1024.0))); - return configuredSize; - } catch (NumberFormatException e) { - throw new IllegalArgumentException( - String.format("Invalid value for system property %s: '%s'. Must be a valid integer.", - BATCH_SIZE_PROPERTY, propertyValue), e); - } -} -``` - -4. **Modify existing constructor** to delegate: -```java -public DocumentUploadQueue(UploadStrategy uploader) { - this(uploader, getConfiguredBatchSize()); -} -``` - -5. **Add new constructor** with maxQueueSize parameter: -```java -/** - * Constructs a new DocumentUploadQueue object with a configurable maximum queue size limit. - * - * @param uploader The upload strategy to be used for document uploads. - * @param maxQueueSize The maximum queue size in bytes. Must not exceed 256MB (Stream API limit). - * @throws IllegalArgumentException if maxQueueSize exceeds the API limit of 256MB. - */ -public DocumentUploadQueue(UploadStrategy uploader, int maxQueueSize) { - if (maxQueueSize > MAX_ALLOWED_QUEUE_SIZE) { - throw new IllegalArgumentException( - String.format("maxQueueSize (%d bytes) exceeds the Stream API limit of %d bytes (256 MB)", - maxQueueSize, MAX_ALLOWED_QUEUE_SIZE)); - } - if (maxQueueSize <= 0) { - throw new IllegalArgumentException("maxQueueSize must be greater than 0"); - } - this.documentToAddList = new ArrayList<>(); - this.documentToDeleteList = new ArrayList<>(); - this.uploader = uploader; - this.maxQueueSize = maxQueueSize; -} -``` - -6. **Update add() methods** to use `this.maxQueueSize` instead of hardcoded value - -**Exact changes for DocumentUploadQueueTest.java**: -- Update test instantiation to use explicit batch size where needed -- Add tests for `getConfiguredBatchSize()` with system property -- Add tests for constructor validation (exceeds 256MB, negative values) - -**Must NOT do**: -- Do NOT include changes to PushService, StreamService, or UpdateStreamService -- Do NOT include StreamDocumentUploadQueue changes -- Do NOT include any documentation files -- Do NOT change the behavioral contract (flush behavior stays the same) - -**PR Title**: `feat(DocumentUploadQueue): add configurable batch size infrastructure` - -**PR Description**: -```markdown -## Summary -Adds infrastructure for configurable batch sizing in DocumentUploadQueue. This is foundational work that enables services to configure their batch size. - -## Changes -- Added constants: `MAX_ALLOWED_QUEUE_SIZE` (256MB), `DEFAULT_QUEUE_SIZE` (256MB), `BATCH_SIZE_PROPERTY` -- Added `getConfiguredBatchSize()` static method for system property configuration -- Added new constructor with `maxQueueSize` parameter -- Added validation to prevent exceeding 256MB API limit -- Updated existing constructor to use `getConfiguredBatchSize()` as default - -## Configuration -- System property: `-Dcoveo.push.batchSize=` -- Constructor parameter: `new DocumentUploadQueue(uploader, maxQueueSize)` -- Priority: constructor > system property > default (256MB) - -## Testing -- Updated existing tests -- Added tests for system property configuration -- Added tests for validation (max size, invalid values) -``` - -**Dependencies**: PR1 must be merged first (for consistent null handling patterns) - -**Recommended Agent Profile**: -- **Category**: `visual-engineering` -- **Skills**: [`git-master`] - - `git-master`: Needed for precise extraction of changes from existing branch - -**Implementation Approach**: -```bash -# 1. Ensure PR1 is merged, then create branch from main -git checkout main -git pull origin main -git checkout -b feat/configurable-batch-size-infrastructure - -# 2. Extract DocumentUploadQueue changes from feature branch -# Option A: Use git show to get the file and manually extract relevant parts -git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java > /tmp/new-queue.java -# Then carefully copy only the infrastructure changes (not behavioral changes) - -# Option B: Apply changes manually based on the spec above - -# 3. Extract test changes -git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java > /tmp/new-test.java -# Copy relevant test updates - -# 4. Verify -mvn clean compile -mvn test -mvn spotless:check - -# 5. Format code -mvn spotless:apply - -# 6. Commit and push -git add src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java -git add src/test/java/com/coveo/pushapiclient/DocumentUploadQueueTest.java -git commit -m "feat(DocumentUploadQueue): add configurable batch size infrastructure" -git push -u origin feat/configurable-batch-size-infrastructure - -# 7. Create PR -gh pr create --title "feat(DocumentUploadQueue): add configurable batch size infrastructure" --body "..." -``` - -**Acceptance Criteria**: -- [ ] `mvn clean compile` succeeds -- [ ] `mvn test` passes -- [ ] `mvn spotless:check` passes -- [ ] `DocumentUploadQueue.getConfiguredBatchSize()` returns 256MB by default -- [ ] System property `coveo.push.batchSize` overrides default -- [ ] Constructor with invalid maxQueueSize throws `IllegalArgumentException` -- [ ] Existing code using `new DocumentUploadQueue(uploader)` still works - -**Verification Script**: -```bash -# Test system property configuration -mvn test -Dtest=DocumentUploadQueueTest -Dcoveo.push.batchSize=134217728 -``` - -**Commit**: YES -- Message: `feat(DocumentUploadQueue): add configurable batch size infrastructure` -- Files: `DocumentUploadQueue.java`, `DocumentUploadQueueTest.java` -- Pre-commit: `mvn test` - ---- - -### TODO 3: Create PR3 - Services + File Container Rotation + Documentation - -- [ ] 3. Create PR3: `feat(services): add configurable batch size with file container rotation` - -**What to do**: - -1. Create new branch `feat/configurable-batch-size-services` from `main` (after PR2 merges) -2. Extract ALL remaining changes from the feature branch -3. This is the largest PR - contains service updates, behavioral changes, tests, and documentation - -**Files to include**: - -| File | Change Type | Specific Changes | -|------|-------------|------------------| -| `src/main/java/com/coveo/pushapiclient/PushService.java` | MODIFY | New constructor with `maxQueueSize` | -| `src/main/java/com/coveo/pushapiclient/StreamService.java` | MODIFY | New constructors with `maxQueueSize`, update existing constructors | -| `src/main/java/com/coveo/pushapiclient/UpdateStreamService.java` | MODIFY | New constructors, remove `fileContainer` field, remove `getUploadStrategy()`, update method bodies | -| `src/main/java/com/coveo/pushapiclient/StreamDocumentUploadQueue.java` | MODIFY | New constructor, `setUpdateStreamService()`, `flushAndPush()`, override `add()` methods | -| `src/main/java/com/coveo/pushapiclient/UpdateStreamServiceInternal.java` | MODIFY | Remove file container creation from `add*()`, add `createUploadAndPush()`, modify `close()` | -| `src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueTest.java` | MODIFY | Update tests for new behavior | -| `src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueBatchingTest.java` | NEW | New test file (274 lines) | -| `src/test/java/com/coveo/pushapiclient/FileContainerRotationIntegrationTest.java` | NEW | New test file (225 lines) | -| `src/test/java/com/coveo/pushapiclient/UpdateStreamServiceInternalTest.java` | MODIFY | Update tests for new behavior | -| `CONFIGURATION.md` | NEW | Configuration guide (235 lines) | -| `UPGRADE_NOTES.md` | NEW | Migration guidance (171 lines) | -| `README.md` | MODIFY | Add configuration section (24 lines) | -| `samples/UpdateStreamDocuments.java` | MODIFY | Add explanatory comments (8 lines) | - -**Detailed changes by file**: - -#### PushService.java -- Add new constructor: `PushService(source, options, int maxQueueSize)` -- Update existing constructors to use `DEFAULT_QUEUE_SIZE` -- Pass `maxQueueSize` to `DocumentUploadQueue` constructor - -#### StreamService.java -- Add new 4-param constructor: `StreamService(source, options, userAgents, int maxQueueSize)` -- Update all existing constructors to delegate to the 4-param version -- Add `userAgents != null` check before calling `setUserAgents()` -- Pass `maxQueueSize` to `DocumentUploadQueue` constructor - -#### UpdateStreamService.java -- Remove `private FileContainer fileContainer;` field -- Add new 4-param constructor with `maxQueueSize` -- Update all existing constructors to delegate to 4-param version -- Add `userAgents != null` check -- Pass `null` for UploadStrategy and `maxQueueSize` to `StreamDocumentUploadQueue` -- Remove `getUploadStrategy()` method -- Update `addOrUpdate()`, `addPartialUpdate()`, `delete()` to not capture return value -- Update Javadoc to reflect new behavior - -#### StreamDocumentUploadQueue.java -- Add field: `private UpdateStreamServiceInternal updateStreamService;` -- Add new constructor: `StreamDocumentUploadQueue(uploader, int maxQueueSize)` -- Add method: `setUpdateStreamService(UpdateStreamServiceInternal)` -- Add method: `flushAndPush()` - creates container, uploads, pushes -- Override `flush()` to check for null uploader -- Override `add(DocumentBuilder)` to call `flushAndPush()` on batch limit -- Override `add(DeleteDocument)` to call `flushAndPush()` on batch limit -- Override `add(PartialUpdateDocument)` to call `flushAndPush()` on batch limit - -#### UpdateStreamServiceInternal.java -- Add `queue.setUpdateStreamService(this);` in constructor -- Remove `if (this.fileContainer == null)` checks from `addOrUpdate()`, `addPartialUpdate()`, `delete()` -- Add new method: `createUploadAndPush(StreamUpdate)` - handles create→upload→push workflow -- Modify `close()` to call `queue.flushAndPush()` instead of `pushFileContainer()` -- Remove `createFileContainer()` private method -- Remove `pushFileContainer()` private method - -**Must NOT do**: -- Do NOT modify DocumentUploadQueue.java (already done in PR2) -- Do NOT modify PlatformClient.java (already done in PR1) -- Do NOT change any logic that wasn't in the original PR - -**PR Title**: `feat(services): add configurable batch size with file container rotation` - -**PR Description**: -```markdown -## Summary -Adds configurable batch size support to all push services and implements per-batch file container rotation for UpdateStreamService following Coveo's catalog stream API best practices. - -## Key Changes - -### Configurable Batch Size -All services now support custom batch sizes (max: 256MB): -- `PushService(source, options, maxQueueSize)` -- `StreamService(source, options, userAgents, maxQueueSize)` -- `UpdateStreamService(source, options, userAgents, maxQueueSize)` - -### File Container Rotation (UpdateStreamService) -Each batch now gets its own file container: -1. Create new file container -2. Upload batch content -3. Push container immediately - -This follows the [catalog stream API best practices](https://docs.coveo.com/en/p4eb0129/coveo-for-commerce/full-catalog-data-updates#update-operations). - -## Breaking Changes -- Default batch size increased from 5MB to 256MB -- UpdateStreamService pushes each batch immediately instead of accumulating - -## Documentation -- Added CONFIGURATION.md with complete configuration guide -- Added UPGRADE_NOTES.md with migration guidance -- Updated README.md with configuration section - -## Testing -- Added StreamDocumentUploadQueueBatchingTest (new) -- Added FileContainerRotationIntegrationTest (new) -- Updated existing tests for new behavior -``` - -**Dependencies**: PR2 must be merged first - -**Recommended Agent Profile**: -- **Category**: `visual-engineering` -- **Skills**: [`git-master`] - - `git-master`: Complex git operations to extract remaining changes - -**Implementation Approach**: -```bash -# 1. Ensure PR2 is merged, then create branch from main -git checkout main -git pull origin main -git checkout -b feat/configurable-batch-size-services - -# 2. Since PR1 and PR2 changes are now in main, we can cherry-pick -# the remaining changes more easily. However, commits are mixed. -# Best approach: copy files from feature branch and let git diff handle it - -# For each source file: -git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/PushService.java > src/main/java/com/coveo/pushapiclient/PushService.java -git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/StreamService.java > src/main/java/com/coveo/pushapiclient/StreamService.java -git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/UpdateStreamService.java > src/main/java/com/coveo/pushapiclient/UpdateStreamService.java -git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/StreamDocumentUploadQueue.java > src/main/java/com/coveo/pushapiclient/StreamDocumentUploadQueue.java -git show feature/configurable-batch-size-256mb:src/main/java/com/coveo/pushapiclient/UpdateStreamServiceInternal.java > src/main/java/com/coveo/pushapiclient/UpdateStreamServiceInternal.java - -# For each test file: -git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueTest.java > src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueTest.java -git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueBatchingTest.java > src/test/java/com/coveo/pushapiclient/StreamDocumentUploadQueueBatchingTest.java -git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/FileContainerRotationIntegrationTest.java > src/test/java/com/coveo/pushapiclient/FileContainerRotationIntegrationTest.java -git show feature/configurable-batch-size-256mb:src/test/java/com/coveo/pushapiclient/UpdateStreamServiceInternalTest.java > src/test/java/com/coveo/pushapiclient/UpdateStreamServiceInternalTest.java - -# For documentation: -git show feature/configurable-batch-size-256mb:CONFIGURATION.md > CONFIGURATION.md -git show feature/configurable-batch-size-256mb:UPGRADE_NOTES.md > UPGRADE_NOTES.md -git show feature/configurable-batch-size-256mb:README.md > README.md -git show feature/configurable-batch-size-256mb:samples/UpdateStreamDocuments.java > samples/UpdateStreamDocuments.java - -# 3. Verify -mvn clean compile -mvn test -mvn spotless:check - -# 4. Format if needed -mvn spotless:apply - -# 5. Stage all changes -git add -A - -# 6. Commit -git commit -m "feat(services): add configurable batch size with file container rotation - -- Add configurable batch size to PushService, StreamService, UpdateStreamService -- Implement per-batch file container rotation for UpdateStreamService -- Add CONFIGURATION.md and UPGRADE_NOTES.md documentation -- Update README.md with configuration section -- Add comprehensive tests for batching and rotation behavior" - -# 7. Push and create PR -git push -u origin feat/configurable-batch-size-services -gh pr create --title "feat(services): add configurable batch size with file container rotation" --body "..." -``` - -**Acceptance Criteria**: - -**Compilation & Tests**: -- [ ] `mvn clean compile` succeeds -- [ ] `mvn test` passes all tests (including new tests) -- [ ] `mvn spotless:check` passes - -**PushService**: -- [ ] New constructor `PushService(source, options, maxQueueSize)` available -- [ ] Default constructor uses 256MB batch size -- [ ] Batching works correctly with custom size - -**StreamService**: -- [ ] New constructor `StreamService(source, options, userAgents, maxQueueSize)` available -- [ ] All existing constructors still work -- [ ] `userAgents` can be null without error - -**UpdateStreamService**: -- [ ] New constructor `UpdateStreamService(source, options, userAgents, maxQueueSize)` available -- [ ] Each batch creates its own file container (verify via logs) -- [ ] Batches are pushed immediately when limit exceeded -- [ ] `close()` pushes remaining documents - -**Documentation**: -- [ ] CONFIGURATION.md exists with complete guide -- [ ] UPGRADE_NOTES.md explains behavioral changes -- [ ] README.md has new Configuration section - -**Commit**: YES -- Message: `feat(services): add configurable batch size with file container rotation` -- Files: 13 files (5 source, 4 test, 4 doc) -- Pre-commit: `mvn test` - ---- - -## Commit Strategy - -| PR | After Merge | Commit Message | Key Files | -|----|-------------|----------------|-----------| -| 1 | PR1 | `fix(PlatformClient): validate userAgents is not null` | PlatformClient.java | -| 2 | PR2 | `feat(DocumentUploadQueue): add configurable batch size infrastructure` | DocumentUploadQueue.java, DocumentUploadQueueTest.java | -| 3 | PR3 | `feat(services): add configurable batch size with file container rotation` | 13 files | - ---- - -## Success Criteria - -### Verification Commands -```bash -# After all PRs merged: -git checkout main -git pull origin main -mvn clean test # Expected: BUILD SUCCESS -mvn spotless:check # Expected: no violations - -# Verify batch size configuration -mvn test -Dcoveo.push.batchSize=134217728 # Should use 128MB -``` - -### Final Checklist -- [ ] PR1 merged and passing CI -- [ ] PR2 merged and passing CI -- [ ] PR3 merged and passing CI -- [ ] Original PR #186 closed (superseded) -- [ ] All 16 files from original PR are accounted for -- [ ] Documentation is complete and accurate -- [ ] Behavioral change (file container rotation) is working - ---- - -## Risks and Mitigations - -| Risk | Likelihood | Impact | Mitigation | -|------|------------|--------|------------| -| Merge conflicts between PRs | Low | Medium | Merge PRs in strict order; don't parallelize | -| PR2 breaks existing code | Low | High | Run full test suite; existing constructor unchanged | -| PR3 tests fail due to missing PR2 infrastructure | Medium | Medium | Ensure PR2 is fully merged before creating PR3 branch | -| Documentation out of sync | Low | Low | Include all docs in PR3 to keep them together | - ---- - -## Post-Merge Cleanup - -After PR3 is merged: -1. Close original PR #186 with comment: "Superseded by #PR1, #PR2, #PR3" -2. Delete feature branch: `git push origin --delete feature/configurable-batch-size-256mb` -3. Delete local branches created for split PRs (optional) From 79cd20198f98ce9cbeb07bf938bf0aeae09df971 Mon Sep 17 00:00:00 2001 From: ylakhdar Date: Thu, 29 Jan 2026 12:52:50 -0500 Subject: [PATCH 4/5] format --- .../pushapiclient/DocumentUploadQueue.java | 68 ++++++++++++------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java b/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java index 87896c64..637404ce 100644 --- a/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java +++ b/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java @@ -5,31 +5,35 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -/** Represents a queue for uploading documents using a specified upload strategy */ +/** + * Represents a queue for uploading documents using a specified upload strategy + */ class DocumentUploadQueue { private static final Logger logger = LogManager.getLogger(DocumentUploadQueue.class); - + /** Maximum allowed queue size based on Stream API limit (256 MB) */ protected static final int MAX_ALLOWED_QUEUE_SIZE = 256 * 1024 * 1024; - + /** Default queue size (5 MB) */ protected static final int DEFAULT_QUEUE_SIZE = 5 * 1024 * 1024; - + /** System property name for configuring the default batch size */ public static final String BATCH_SIZE_PROPERTY = "coveo.push.batchSize"; - + protected UploadStrategy uploader; protected final int maxQueueSize; protected ArrayList documentToAddList; protected ArrayList documentToDeleteList; protected int size; - + /** * Validates batch size against constraints (> 0 and <= 256MB). - * Used by getConfiguredBatchSize and constructors to ensure consistent validation logic. + * Used by getConfiguredBatchSize and constructors to ensure consistent + * validation logic. * * @param sizeBytes The batch size in bytes to validate - * @throws IllegalArgumentException if size exceeds MAX_ALLOWED_QUEUE_SIZE or is <= 0 + * @throws IllegalArgumentException if size exceeds MAX_ALLOWED_QUEUE_SIZE or is + * <= 0 */ protected static void validateBatchSize(int sizeBytes) { if (sizeBytes > MAX_ALLOWED_QUEUE_SIZE) { @@ -44,35 +48,40 @@ protected static void validateBatchSize(int sizeBytes) { } /** - * Gets the configured batch size from system properties, or returns the default if not set. + * Gets the configured batch size from system properties, or returns the default + * if not set. * - * The system property is read as bytes. When not set, returns DEFAULT_QUEUE_SIZE (5 MB). + * The system property is read as bytes. When not set, returns + * DEFAULT_QUEUE_SIZE (5 MB). * * Example: Set a 50 MB batch size via system property: + * * 
    *   java -Dcoveo.push.batchSize=52428800 -jar app.jar  // 50 * 1024 * 1024 bytes
    *
* * @return The configured batch size in bytes (e.g., 52428800 for 50 MB) - * @throws IllegalArgumentException if the configured value exceeds 256MB or is invalid + * @throws IllegalArgumentException if the configured value exceeds 256MB or is + * invalid */ public static int getConfiguredBatchSize() { String propertyValue = System.getProperty(BATCH_SIZE_PROPERTY); if (propertyValue == null || propertyValue.trim().isEmpty()) { return DEFAULT_QUEUE_SIZE; } - + int configuredSize; try { configuredSize = Integer.parseInt(propertyValue.trim()); } catch (NumberFormatException e) { throw new IllegalArgumentException( String.format("Invalid value for system property %s: '%s'. Must be a valid integer.", - BATCH_SIZE_PROPERTY, propertyValue), e); + BATCH_SIZE_PROPERTY, propertyValue), + e); } - + validateBatchSize(configuredSize); - + logger.info(String.format("Using configured batch size from system property %s: %d bytes (%.2f MB)", BATCH_SIZE_PROPERTY, configuredSize, configuredSize / (1024.0 * 1024.0))); return configuredSize; @@ -81,22 +90,27 @@ public static int getConfiguredBatchSize() { /** * Constructs a new DocumentUploadQueue with the default batch size. * - * Uses the configured batch size from system property "coveo.push.batchSize" if set, + * Uses the configured batch size from system property "coveo.push.batchSize" if + * set, * otherwise defaults to DEFAULT_QUEUE_SIZE (5 MB = 5242880 bytes). * * @param uploader The upload strategy to be used for document uploads. - * @throws IllegalArgumentException if the system property value exceeds 256MB or is invalid. + * @throws IllegalArgumentException if the system property value exceeds 256MB + * or is invalid. */ public DocumentUploadQueue(UploadStrategy uploader) { this(uploader, getConfiguredBatchSize()); } /** - * Constructs a new DocumentUploadQueue object with a configurable maximum queue size limit. + * Constructs a new DocumentUploadQueue object with a configurable maximum queue + * size limit. * - * @param uploader The upload strategy to be used for document uploads. - * @param maxQueueSize The maximum queue size in bytes (e.g., 52428800 for 50 MB). Must not exceed 256MB (Stream API limit). - * @throws IllegalArgumentException if maxQueueSize exceeds the API limit of 256MB. + * @param uploader The upload strategy to be used for document uploads. + * @param maxQueueSize The maximum queue size in bytes (e.g., 52428800 for 50 + * MB). Must not exceed 256MB (Stream API limit). + * @throws IllegalArgumentException if maxQueueSize exceeds the API limit of + * 256MB. */ public DocumentUploadQueue(UploadStrategy uploader, int maxQueueSize) { validateBatchSize(maxQueueSize); @@ -119,7 +133,7 @@ public DocumentUploadQueue() { /** * Flushes the accumulated documents by applying the upload strategy. * - * @throws IOException If an I/O error occurs during the upload. + * @throws IOException If an I/O error occurs during the upload. * @throws InterruptedException If the upload process is interrupted. */ public void flush() throws IOException, InterruptedException { @@ -138,11 +152,12 @@ public void flush() throws IOException, InterruptedException { } /** - * Adds a {@link DocumentBuilder} to the upload queue and flushes the queue if it exceeds the + * Adds a {@link DocumentBuilder} to the upload queue and flushes the queue if + * it exceeds the * maximum content length. See {@link DocumentUploadQueue#flush}. * * @param document The document to be added to the index. - * @throws IOException If an I/O error occurs during the upload. + * @throws IOException If an I/O error occurs during the upload. * @throws InterruptedException If the upload process is interrupted. */ public void add(DocumentBuilder document) throws IOException, InterruptedException { @@ -162,11 +177,12 @@ public void add(DocumentBuilder document) throws IOException, InterruptedExcepti } /** - * Adds the {@link DeleteDocument} to the upload queue and flushes the queue if it exceeds the + * Adds the {@link DeleteDocument} to the upload queue and flushes the queue if + * it exceeds the * maximum content length. See {@link DocumentUploadQueue#flush}. * * @param document The document to be deleted from the index. - * @throws IOException If an I/O error occurs during the upload. + * @throws IOException If an I/O error occurs during the upload. * @throws InterruptedException If the upload process is interrupted. */ public void add(DeleteDocument document) throws IOException, InterruptedException { From 38e20b6149c65ddf449da033d9b98c7808dc58f3 Mon Sep 17 00:00:00 2001 From: ylakhdar Date: Thu, 29 Jan 2026 13:21:52 -0500 Subject: [PATCH 5/5] run spotless --- .../pushapiclient/DocumentUploadQueue.java | 80 ++++++++----------- 1 file changed, 35 insertions(+), 45 deletions(-) diff --git a/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java b/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java index 637404ce..1862039f 100644 --- a/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java +++ b/src/main/java/com/coveo/pushapiclient/DocumentUploadQueue.java @@ -5,9 +5,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -/** - * Represents a queue for uploading documents using a specified upload strategy - */ +/** Represents a queue for uploading documents using a specified upload strategy */ class DocumentUploadQueue { private static final Logger logger = LogManager.getLogger(DocumentUploadQueue.class); @@ -27,13 +25,11 @@ class DocumentUploadQueue { protected int size; /** - * Validates batch size against constraints (> 0 and <= 256MB). - * Used by getConfiguredBatchSize and constructors to ensure consistent - * validation logic. - * + * Validates batch size against constraints (> 0 and <= 256MB). Used by getConfiguredBatchSize and + * constructors to ensure consistent validation logic. + * * @param sizeBytes The batch size in bytes to validate - * @throws IllegalArgumentException if size exceeds MAX_ALLOWED_QUEUE_SIZE or is - * <= 0 + * @throws IllegalArgumentException if size exceeds MAX_ALLOWED_QUEUE_SIZE or is <= 0 */ protected static void validateBatchSize(int sizeBytes) { if (sizeBytes > MAX_ALLOWED_QUEUE_SIZE) { @@ -48,21 +44,18 @@ protected static void validateBatchSize(int sizeBytes) { } /** - * Gets the configured batch size from system properties, or returns the default - * if not set. - * - * The system property is read as bytes. When not set, returns - * DEFAULT_QUEUE_SIZE (5 MB). - * - * Example: Set a 50 MB batch size via system property: - * + * Gets the configured batch size from system properties, or returns the default if not set. + * + *

The system property is read as bytes. When not set, returns DEFAULT_QUEUE_SIZE (5 MB). + * + *

Example: Set a 50 MB batch size via system property: + * * 

    *   java -Dcoveo.push.batchSize=52428800 -jar app.jar  // 50 * 1024 * 1024 bytes
    *
- * + * * @return The configured batch size in bytes (e.g., 52428800 for 50 MB) - * @throws IllegalArgumentException if the configured value exceeds 256MB or is - * invalid + * @throws IllegalArgumentException if the configured value exceeds 256MB or is invalid */ public static int getConfiguredBatchSize() { String propertyValue = System.getProperty(BATCH_SIZE_PROPERTY); @@ -75,42 +68,41 @@ public static int getConfiguredBatchSize() { configuredSize = Integer.parseInt(propertyValue.trim()); } catch (NumberFormatException e) { throw new IllegalArgumentException( - String.format("Invalid value for system property %s: '%s'. Must be a valid integer.", + String.format( + "Invalid value for system property %s: '%s'. Must be a valid integer.", BATCH_SIZE_PROPERTY, propertyValue), e); } validateBatchSize(configuredSize); - logger.info(String.format("Using configured batch size from system property %s: %d bytes (%.2f MB)", - BATCH_SIZE_PROPERTY, configuredSize, configuredSize / (1024.0 * 1024.0))); + logger.info( + String.format( + "Using configured batch size from system property %s: %d bytes (%.2f MB)", + BATCH_SIZE_PROPERTY, configuredSize, configuredSize / (1024.0 * 1024.0))); return configuredSize; } /** * Constructs a new DocumentUploadQueue with the default batch size. - * - * Uses the configured batch size from system property "coveo.push.batchSize" if - * set, - * otherwise defaults to DEFAULT_QUEUE_SIZE (5 MB = 5242880 bytes). + * + *

Uses the configured batch size from system property "coveo.push.batchSize" if set, otherwise + * defaults to DEFAULT_QUEUE_SIZE (5 MB = 5242880 bytes). * * @param uploader The upload strategy to be used for document uploads. - * @throws IllegalArgumentException if the system property value exceeds 256MB - * or is invalid. + * @throws IllegalArgumentException if the system property value exceeds 256MB or is invalid. */ public DocumentUploadQueue(UploadStrategy uploader) { this(uploader, getConfiguredBatchSize()); } /** - * Constructs a new DocumentUploadQueue object with a configurable maximum queue - * size limit. + * Constructs a new DocumentUploadQueue object with a configurable maximum queue size limit. * - * @param uploader The upload strategy to be used for document uploads. - * @param maxQueueSize The maximum queue size in bytes (e.g., 52428800 for 50 - * MB). Must not exceed 256MB (Stream API limit). - * @throws IllegalArgumentException if maxQueueSize exceeds the API limit of - * 256MB. + * @param uploader The upload strategy to be used for document uploads. + * @param maxQueueSize The maximum queue size in bytes (e.g., 52428800 for 50 MB). Must not exceed + * 256MB (Stream API limit). + * @throws IllegalArgumentException if maxQueueSize exceeds the API limit of 256MB. */ public DocumentUploadQueue(UploadStrategy uploader, int maxQueueSize) { validateBatchSize(maxQueueSize); @@ -121,8 +113,8 @@ public DocumentUploadQueue(UploadStrategy uploader, int maxQueueSize) { } /** - * Default constructor for testing purposes (used by Mockito @InjectMocks). - * Initializes with default batch size; uploader is injected by Mockito. + * Default constructor for testing purposes (used by Mockito @InjectMocks). Initializes with + * default batch size; uploader is injected by Mockito. */ public DocumentUploadQueue() { this.documentToAddList = new ArrayList<>(); @@ -133,7 +125,7 @@ public DocumentUploadQueue() { /** * Flushes the accumulated documents by applying the upload strategy. * - * @throws IOException If an I/O error occurs during the upload. + * @throws IOException If an I/O error occurs during the upload. * @throws InterruptedException If the upload process is interrupted. */ public void flush() throws IOException, InterruptedException { @@ -152,12 +144,11 @@ public void flush() throws IOException, InterruptedException { } /** - * Adds a {@link DocumentBuilder} to the upload queue and flushes the queue if - * it exceeds the + * Adds a {@link DocumentBuilder} to the upload queue and flushes the queue if it exceeds the * maximum content length. See {@link DocumentUploadQueue#flush}. * * @param document The document to be added to the index. - * @throws IOException If an I/O error occurs during the upload. + * @throws IOException If an I/O error occurs during the upload. * @throws InterruptedException If the upload process is interrupted. */ public void add(DocumentBuilder document) throws IOException, InterruptedException { @@ -177,12 +168,11 @@ public void add(DocumentBuilder document) throws IOException, InterruptedExcepti } /** - * Adds the {@link DeleteDocument} to the upload queue and flushes the queue if - * it exceeds the + * Adds the {@link DeleteDocument} to the upload queue and flushes the queue if it exceeds the * maximum content length. See {@link DocumentUploadQueue#flush}. * * @param document The document to be deleted from the index. - * @throws IOException If an I/O error occurs during the upload. + * @throws IOException If an I/O error occurs during the upload. * @throws InterruptedException If the upload process is interrupted. */ public void add(DeleteDocument document) throws IOException, InterruptedException {