Fix secondary launch log upload#296
Conversation
WalkthroughRefactors launch lifecycle and log emission (renamed emitter factory, new start/completeLogEmitter, additional constructors), reorders PrimaryLaunch.finish cleanup, changes SecondaryLaunch.finish to aggregate and wait on all children then flush logs, adds minor utilities/formatting edits and a CHANGELOG entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant LaunchImpl
participant LogEmitter
participant PrimaryLaunch
participant SecondaryLaunch
participant RP as ReportPortalClient
rect rgba(230,245,255,0.6)
Caller->>LaunchImpl: start() / start(statistics)
LaunchImpl->>LaunchImpl: lazily start launch (Maybe<String>) with caching
LaunchImpl->>LogEmitter: createLogEmitter(...) -- create & subscribe batched emitter
end
rect rgba(255,250,230,0.6)
Caller->>PrimaryLaunch: finish(rq)
PrimaryLaunch->>LaunchImpl: super.finish(rq)
LaunchImpl->>LogEmitter: completeLogEmitter() -- flush remaining batches
PrimaryLaunch-->>PrimaryLaunch: stopRunning(), lock.finishInstanceUuid(uuid) -- moved after super.finish
end
rect rgba(240,255,240,0.6)
Caller->>SecondaryLaunch: finish(rq)
SecondaryLaunch->>SecondaryLaunch: concat all queue children -> Completable
SecondaryLaunch->>SecondaryLaunch: waitForItemsCompletion(...)
SecondaryLaunch->>LaunchImpl: completeLogEmitter()
LaunchImpl->>LogEmitter: flush remaining batched logs
SecondaryLaunch-->>SecondaryLaunch: stopRunning(), lock.finishInstanceUuid(uuid)
end
RP-->>LogEmitter: acknowledgements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/epam/reportportal/utils/files/ImageConverter.java (1)
55-66: Close the input stream and guard against null BufferedImageImageIO.read(InputStream) does not close the stream and may return null for unsupported formats. Current code risks a resource leak and NPE on image.getWidth(...).
Apply this diff:
- BufferedImage image = ImageIO.read(source.openBufferedStream()); - final BufferedImage blackAndWhiteImage = new BufferedImage( - image.getWidth(null), - image.getHeight(null), - BufferedImage.TYPE_BYTE_GRAY - ); + try (java.io.InputStream in = source.openBufferedStream()) { + BufferedImage image = ImageIO.read(in); + if (image == null) { + throw new IOException("Unsupported or unreadable image content"); + } + final BufferedImage blackAndWhiteImage = new BufferedImage( + image.getWidth(), + image.getHeight(), + BufferedImage.TYPE_BYTE_GRAY + ); + final Graphics2D graphics2D = (Graphics2D) blackAndWhiteImage.getGraphics(); + graphics2D.drawImage(image, 0, 0, null); + graphics2D.dispose(); + return convertToInputStream(blackAndWhiteImage); + } - final Graphics2D graphics2D = (Graphics2D) blackAndWhiteImage.getGraphics(); - graphics2D.drawImage(image, 0, 0, null); - graphics2D.dispose(); - return convertToInputStream(blackAndWhiteImage);
🧹 Nitpick comments (3)
src/main/java/com/epam/reportportal/utils/AttributeParser.java (1)
151-152: Trim values before creating attributesPre-trimming avoids persisting leading/trailing spaces while preserving existing blank filtering.
Apply this diff:
- return Arrays.stream(values).filter(StringUtils::isNotBlank).map(v -> createItemAttribute(key, v)).collect(Collectors.toList()); + return Arrays.stream(values) + .filter(Objects::nonNull) + .map(String::trim) + .filter(StringUtils::isNotBlank) + .map(v -> createItemAttribute(key, v)) + .collect(Collectors.toList());src/test/java/com/epam/reportportal/service/step/ManualNestedStepTest.java (1)
331-331: Minor: make test more robust to unrelated logs.Consider asserting the presence of the expected pair rather than exact size to reduce incidental flakiness on slower CI.
- assertThat(logRequests, hasSize(1)); - Pair<String, String> log = logRequests.get(0); - assertThat(log.getKey(), equalTo(LogLevel.DEBUG.name())); - assertThat(log.getValue(), equalTo(logMessage)); + assertThat(logRequests, hasItem(equalTo(Pair.of(LogLevel.DEBUG.name(), logMessage))));src/main/java/com/epam/reportportal/utils/formatting/templating/TemplateProcessing.java (1)
153-155: LGTM: clearer single‑line warning.Optional micro‑nit:
String.join(templateConfig.getFieldDelimiter(), fields)reads a bit cleaner than stream + collector.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
CHANGELOG.md(1 hunks)src/main/java/com/epam/reportportal/aspect/StepNameUtils.java(2 hunks)src/main/java/com/epam/reportportal/listeners/LogLevel.java(1 hunks)src/main/java/com/epam/reportportal/service/LaunchImpl.java(11 hunks)src/main/java/com/epam/reportportal/service/launch/PrimaryLaunch.java(1 hunks)src/main/java/com/epam/reportportal/service/launch/SecondaryLaunch.java(2 hunks)src/main/java/com/epam/reportportal/service/logs/LogBatchingFlowable.java(0 hunks)src/main/java/com/epam/reportportal/utils/AttributeParser.java(1 hunks)src/main/java/com/epam/reportportal/utils/ClientIdProvider.java(0 hunks)src/main/java/com/epam/reportportal/utils/ParameterUtils.java(3 hunks)src/main/java/com/epam/reportportal/utils/TestCaseIdUtils.java(6 hunks)src/main/java/com/epam/reportportal/utils/Waiter.java(1 hunks)src/main/java/com/epam/reportportal/utils/files/ImageConverter.java(1 hunks)src/main/java/com/epam/reportportal/utils/formatting/ExceptionUtils.java(2 hunks)src/main/java/com/epam/reportportal/utils/formatting/MarkdownUtils.java(2 hunks)src/main/java/com/epam/reportportal/utils/formatting/templating/TemplateConfiguration.java(1 hunks)src/main/java/com/epam/reportportal/utils/formatting/templating/TemplateProcessing.java(1 hunks)src/main/java/com/epam/reportportal/utils/http/ClientUtils.java(1 hunks)src/main/java/com/epam/reportportal/utils/http/HttpRequestUtils.java(3 hunks)src/main/java/com/epam/reportportal/utils/properties/OutputTypes.java(1 hunks)src/main/java/com/epam/reportportal/utils/properties/PropertiesLoader.java(4 hunks)src/test/java/com/epam/reportportal/service/step/ManualNestedStepTest.java(3 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/com/epam/reportportal/service/logs/LogBatchingFlowable.java
- src/main/java/com/epam/reportportal/utils/ClientIdProvider.java
🧰 Additional context used
🧬 Code graph analysis (5)
src/main/java/com/epam/reportportal/aspect/StepNameUtils.java (1)
src/main/java/com/epam/reportportal/utils/formatting/templating/TemplateProcessing.java (1)
TemplateProcessing(40-272)
src/test/java/com/epam/reportportal/service/step/ManualNestedStepTest.java (2)
src/main/java/com/epam/reportportal/utils/StaticStructuresUtils.java (1)
StaticStructuresUtils(26-112)src/test/java/com/epam/reportportal/test/TestUtils.java (1)
TestUtils(48-347)
src/main/java/com/epam/reportportal/utils/TestCaseIdUtils.java (2)
src/main/java/com/epam/reportportal/service/item/TestCaseIdEntry.java (1)
TestCaseIdEntry(24-61)src/main/java/com/epam/reportportal/utils/formatting/templating/TemplateProcessing.java (1)
TemplateProcessing(40-272)
src/main/java/com/epam/reportportal/utils/http/HttpRequestUtils.java (1)
src/main/java/com/epam/reportportal/utils/files/ByteSource.java (1)
ByteSource(30-107)
src/main/java/com/epam/reportportal/service/LaunchImpl.java (1)
src/main/java/com/epam/reportportal/utils/StaticStructuresUtils.java (1)
StaticStructuresUtils(26-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (26)
src/main/java/com/epam/reportportal/utils/Waiter.java (1)
122-126: Javadoc touch-up LGTMAccurately reflects current behavior; no code changes.
CHANGELOG.md (1)
4-5: Changelog entry reads wellClear and consistent with prior entries.
src/main/java/com/epam/reportportal/utils/formatting/MarkdownUtils.java (1)
92-93: Formatting-only tweaks look goodStream and spacing changes are no-ops; readability is fine.
Also applies to: 136-137
src/main/java/com/epam/reportportal/listeners/LogLevel.java (1)
23-29: Enum formatting LGTMNo behavioral changes; aligns with enum style elsewhere.
src/main/java/com/epam/reportportal/utils/properties/OutputTypes.java (1)
22-23: Enum formatting LGTMStyle-only; no API changes.
src/main/java/com/epam/reportportal/utils/http/HttpRequestUtils.java (2)
64-72: Multipart parts reflow is a no-opOnly formatting; deprecated RequestBody.create kept for back-compat as noted.
Also applies to: 89-94
21-21: Approve — keep project's ByteSource importNo occurrences of com.google.common.io.ByteSource were found in the repository; using com.epam.reportportal.utils.files.ByteSource is correct.
src/main/java/com/epam/reportportal/utils/formatting/templating/TemplateConfiguration.java (1)
81-84: Formatting-only change looks good.The
equalsmethod has been reformatted for better readability while maintaining the same logic and field comparisons.src/main/java/com/epam/reportportal/utils/formatting/ExceptionUtils.java (1)
44-45: Formatting-only change looks good.The stream operation has been reformatted across multiple lines for better readability without changing the logic.
src/main/java/com/epam/reportportal/utils/http/ClientUtils.java (1)
46-62: Formatting improvements enhance readability.The method signature and lambda expressions have been reformatted to single lines, making the code more concise while preserving functionality.
src/main/java/com/epam/reportportal/utils/TestCaseIdUtils.java (2)
83-83: Loop declaration condensed appropriately.The multi-line for-loop declaration has been collapsed to a single line, improving code compactness without affecting readability.
157-160: Stream operations reformatted consistently.The parameter mapping and template processing calls have been consolidated to single lines, maintaining consistency with the project's formatting style.
src/main/java/com/epam/reportportal/utils/ParameterUtils.java (1)
149-152: Stream concatenation reformatted for clarity.The
Stream.concatoperation has been reformatted with better indentation, making the structure of the concatenated streams more apparent.src/main/java/com/epam/reportportal/utils/properties/PropertiesLoader.java (1)
94-95: Nested Optional operations condensed effectively.The fallback chain for property resolution has been reformatted to a more compact form while maintaining readability.
src/main/java/com/epam/reportportal/service/LaunchImpl.java (7)
144-145: Good refactoring: renamed method improves clarity.Renaming
getLogEmittertocreateLogEmitterbetter reflects that this method creates a new emitter rather than retrieving an existing one.
163-195: Well-structured new constructors expand API flexibility.The addition of protected constructors provides better flexibility for subclasses while maintaining clean separation of concerns.
439-453: Critical fix: NewcompleteLogEmitter()method ensures log delivery.This is the key fix for the secondary launch log upload issue. The method properly:
- Checks if the emitter is already complete
- Sends a final log message to flush any pending batches
- Waits for the last batch to be processed before completing
- Uses a timeout to prevent indefinite blocking
This ensures all logs are sent before JVM exit, which was the core issue being addressed.
493-493: Good integration of the new log completion mechanism.The
finishmethod now properly callscompleteLogEmitter()to ensure all logs are flushed before terminating, which directly addresses the PR's objective.
953-986: Well-designed public API additions to TreeItem.The new public methods provide necessary access for subclasses while maintaining encapsulation:
withParent()- Builder pattern for setting parentaddToQueue()- Controlled child additiongetChildren()- Safe copy returngetParent()- Simple getterThese changes appear to support the reworked SecondaryLaunch.finish logic mentioned in the summary.
996-998: Good atomic operation encapsulation.The new
getOrCompute()method properly encapsulates the atomiccomputeIfAbsentoperation, providing a cleaner API for the concurrent map usage.
417-421: Verify visibility change impact on existing consumers.Method completeLogCompletables() was changed from public to private — confirm no external callers or downstream modules depend on it.
Location: src/main/java/com/epam/reportportal/service/LaunchImpl.java (≈ lines 417–421)
private Completable completeLogCompletables() { Completable items = Completable.merge(logCompletables); logCompletables.clear(); return items; }Run locally to verify:
- git grep -n "completeLogCompletables" || true
- rg -n "completeLogCompletables" --hidden -S -g '!/test/' || true
- rg -n '"completeLogCompletables"' -S || true # catch reflection/string usages
If any external usage exists, restore visibility or update callers.
src/test/java/com/epam/reportportal/service/step/ManualNestedStepTest.java (3)
27-27: LGTM: uses shared constant for filtering.Switching to
StaticStructuresUtils.LAUNCH_FINISHED_MESSAGEavoids string drift between prod and tests.
297-297: LGTM: reduced suppression scope.Dropping
ResultOfMethodCallIgnoredis correct after removingblockingAwait(...).
325-325: Finish instead of manual log awaits — good.Calling
launch.finish(...)aligns the test with the new flushing path.src/main/java/com/epam/reportportal/service/launch/SecondaryLaunch.java (1)
38-38: LGTM: required import.src/main/java/com/epam/reportportal/aspect/StepNameUtils.java (1)
49-50: LGTM: formatting‑only, behavior preserved.Also applies to: 72-73, 80-80
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/epam/reportportal/service/LaunchImpl.java (1)
349-352: Align executor-shutdown handling: throw or return Maybe.empty()Found: LaunchImpl constructors throw InternalReportPortalClientException when given a shutdown ExecutorService (src/main/java/com/epam/reportportal/service/LaunchImpl.java:178–182 and 222–226), but start() logs and returns Maybe.empty() when the executor is shut down (src/main/java/com/epam/reportportal/service/LaunchImpl.java:349–352). The codebase also uses Maybe.empty() for NOOP defaults (e.g., src/main/java/com/epam/reportportal/service/Launch.java:244).
Action (choose one):
- Make start() throw InternalReportPortalClientException to match constructor precondition checks — update callers/tests.
- Or keep start() returning Maybe.empty() and document runtime-no-op behavior in Javadoc; ensure all callers tolerate empty Maybe.
Apply the chosen approach consistently across LaunchImpl and related APIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/epam/reportportal/service/LaunchImpl.java(11 hunks)src/main/java/com/epam/reportportal/service/launch/PrimaryLaunch.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/epam/reportportal/service/launch/PrimaryLaunch.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/epam/reportportal/service/LaunchImpl.java (1)
src/main/java/com/epam/reportportal/utils/StaticStructuresUtils.java (1)
StaticStructuresUtils(26-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (12)
src/main/java/com/epam/reportportal/service/LaunchImpl.java (12)
144-156: LGTM! Method renamed for clarityThe rename from
getLogEmittertocreateLogEmitterbetter reflects that this method creates and configures a new log emitter rather than simply retrieving an existing one.
163-194: Enhanced constructor with comprehensive documentationThe constructor documentation clearly explains the parameters and their purposes. The validation of the executor service shutdown state is appropriate to prevent issues with resource management.
196-207: Clean convenience constructor implementationThe constructor provides a sensible default by using the standard
LoggingSubscriber, reducing boilerplate for common use cases.
209-236: Well-documented constructor for existing launch scenariosThe constructor properly handles the case where a launch ID is already available, with appropriate documentation explaining the behavior difference.
244-253: Good scheduler management with proper documentationThe method documentation clearly explains the one-scheduler-per-executor mapping, which is important for understanding the resource management strategy.
284-291: Added getter for statistics servicePackage-private visibility is appropriate for internal access to the statistics service.
340-365: Enhanced start method with statistics controlThe method provides good error handling for shutdown executors and allows fine-grained control over statistics reporting.
417-421: Method visibility changed to privateThe method
completeLogCompletables()has been changed from public to private visibility, which is appropriate since it's an internal implementation detail.
434-453: New centralized log emitter completion methodThis method provides a clean way to finalize batched log uploads with proper timeout handling. The implementation ensures the last log batch is sent and waits for processing completion.
460-495: Improved finish method with proper log completionThe finish method now properly calls
completeLogEmitter()at the end, ensuring all logs are flushed before the launch completes. The error handling for shutdown executors is consistent with the start method.
948-957: Enhanced TreeItem documentationThe TreeItem class methods now have comprehensive Javadoc documentation that clearly explains their purpose and behavior, improving code maintainability.
Also applies to: 959-966, 968-976, 978-986
990-999: Well-documented ComputationConcurrentHashMap methodThe
getOrComputemethod documentation clearly explains its atomic behavior for getting or creating TreeItem instances.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/epam/reportportal/utils/formatting/templating/TemplateProcessing.java (1)
105-107: Log the exception for diagnosabilityInclude the caught throwable in the log to retain the stack trace.
- LOGGER.error("Unable to parse: {}", templatePart); + LOGGER.error("Unable to parse: {}", templatePart, e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/epam/reportportal/utils/files/ImageConverter.java(1 hunks)src/main/java/com/epam/reportportal/utils/formatting/templating/TemplateProcessing.java(1 hunks)src/test/java/com/epam/reportportal/service/step/ManualNestedStepTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/epam/reportportal/utils/files/ImageConverter.java
- src/test/java/com/epam/reportportal/service/step/ManualNestedStepTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
src/main/java/com/epam/reportportal/utils/formatting/templating/TemplateProcessing.java (1)
151-153: Good simplification: replace stream join with String.joinSemantics are unchanged; simpler and marginally cheaper. Please confirm
templateConfig.getFieldDelimiter()is guaranteed non-null (same requirement as before).
1 participant
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Style
Reliability