Merged
Conversation
Contributor
Reviewer's Guide by SourceryThis pull request focuses on significantly improving the documentation of the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @p14n - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider updating the Javadoc exclusion path in
build.gradlefrom**/grpc/**to reflect the package rename toremote, or remove it if no longer needed.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| .build()); | ||
| } | ||
|
|
||
| /** |
Contributor
There was a problem hiding this comment.
suggestion: Review the commented-out subscriber cleanup logic.
If removal of the topic from the 'subscribed' set is intended upon error, consider uncommenting it; otherwise, remove the dead code to keep the implementation clear.
| * @param topic the topic to associate with this event | ||
| * @return this event instance with the topic set | ||
| */ | ||
| public SystemEvent withTopic(String topic) { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Avoid mutating enum instances in the withTopic method.
Since enum values are constants in Java, modifying the 'topic' field directly can lead to unexpected behavior and thread-safety issues. Consider returning a new data structure or using an immutable pattern to associate topics with events.
| [](https://github.com/p14n/postevent/actions) | ||
| [](LICENSE) | ||
| [](https://github.com/yourusername/postevent/releases) | ||
| [](https://github.com/yourusername/p14n/releases) |
Contributor
There was a problem hiding this comment.
issue (typo): Incorrect link URL for "Latest Release" badge.
The link points to .../yourusername/p14n/releases but should likely point to .../p14n/postevent/releases to match the badge source repository and the project's standard repository URL.
Suggested change
| [](https://github.com/yourusername/p14n/releases) | |
| [](https://github.com/p14n/postevent/releases) |
| for (MessageSubscriber<TransactionalEvent> subscriber : topicSubscribers.get(topic)) { | ||
| try (Connection c = ds.getConnection()) { | ||
|
|
||
| processWithTelemetry(openTelemetry, tracer, message, "ordered_process", () -> { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the inner telemetry-wrapped logic into a helper method to simplify the nested lambda structure.
Consider extracting the inner telemetry-wrapped logic into helper methods to reduce nested lambda complexity. For example, you can refactor the inner processing logic into a separate method:
private boolean processMessage(
MessageSubscriber<TransactionalEvent> subscriber,
String topic,
Connection connection,
Event event
) {
return processWithTelemetry(openTelemetry, tracer, event, "message_transaction", () -> {
try {
subscriber.onMessage(new TransactionalEvent(connection, event));
metrics.recordReceived(topic);
return true;
} catch (Exception e) {
try {
subscriber.onError(e);
} catch (Exception ignored) {
}
return false;
}
});
}Then update the publish method to use a single telemetry wrapper for the OrderedProcessor:
@Override
public void publish(String topic, Event message) {
if (!canProcess(topic, message)) {
return;
}
metrics.recordPublished(topic);
for (MessageSubscriber<TransactionalEvent> subscriber : topicSubscribers.get(topic)) {
try (Connection c = ds.getConnection()) {
processWithTelemetry(openTelemetry, tracer, message, "ordered_process", () -> {
var op = new OrderedProcessor(systemEventBroker, (connection, event) ->
processMessage(subscriber, topic, connection, event)
);
op.process(c, message);
return null;
});
} catch (Exception e) {
try {
subscriber.onError(e);
} catch (Exception ignored) {
// If error handling fails, we ignore it to protect other subscribers
}
}
}
}This approach maintains all functionality while reducing nested lambda complexity by isolating the inner logic into its own helper method.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Prepare the library for release by adding Maven publishing configuration and improving internal documentation.
Build:
maven-publishplugin and configure coordinates, POM details, and signing for publishing to Maven Central.1.0.0-SNAPSHOT.CI:
publish.yml) to automate publishing to Maven Central upon release creation.Documentation:
Tests:
Chores:
*.grpcto*.remote.