-
Notifications
You must be signed in to change notification settings - Fork 0
Feature implementation from commits acb40e1..e359f1b #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature-base-2
Are you sure you want to change the base?
Conversation
iluwatar#3252) Bumps [org.jacoco:jacoco-maven-plugin](https://github.com/jacoco/jacoco) from 0.8.12 to 0.8.13. - [Release notes](https://github.com/jacoco/jacoco/releases) - [Commits](jacoco/jacoco@v0.8.12...v0.8.13) --- updated-dependencies: - dependency-name: org.jacoco:jacoco-maven-plugin dependency-version: 0.8.13 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…atar#3253) Bumps [io.micrometer:micrometer-tracing-bridge-brave](https://github.com/micrometer-metrics/tracing) from 1.4.4 to 1.4.5. - [Release notes](https://github.com/micrometer-metrics/tracing/releases) - [Commits](micrometer-metrics/tracing@v1.4.4...v1.4.5) --- updated-dependencies: - dependency-name: io.micrometer:micrometer-tracing-bridge-brave dependency-version: 1.4.5 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ar#3254) Bumps [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) from 2.44.3 to 2.44.4. - [Release notes](https://github.com/diffplug/spotless/releases) - [Changelog](https://github.com/diffplug/spotless/blob/main/CHANGES.md) - [Commits](diffplug/spotless@maven/2.44.3...maven/2.44.4) --- updated-dependencies: - dependency-name: com.diffplug.spotless:spotless-maven-plugin dependency-version: 2.44.4 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* feat: Implement Actor Model pattern iluwatar#3232 * feat: Implement Actor Model pattern iluwatar#3232 * feat: update Actor Model implementation with multi-actor logic iluwatar#3251 * feat: update Actor Model implementation with multi-actor logic and loose coupling iluwatar#3251 * test: add unit test for actor model iluwatar#3251 * test: add test for App.java to increase coverage * docs: add complete README for Actor Model pattern also implemented changes iluwatar#3251
* docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
|
|
||
| // rather than being cached in a thread's local memory. To make it consistent to all Actors | ||
|
|
||
| public void send(Message message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Null message handling missing in send().
The send() method doesn't check for null messages, which will cause NullPointerException when processing the message in onReceive().
Current Code (Diff):
- public void send(Message message) {
- mailbox.add(message); // Add message to queue
- }
+ public void send(Message message) {
+ if (message == null) {
+ throw new IllegalArgumentException("Message cannot be null");
+ }
+ mailbox.add(message); // Add message to queue
+ }📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| public void send(Message message) { | |
| public void send(Message message) { | |
| if (message == null) { | |
| throw new IllegalArgumentException("Message cannot be null"); | |
| } | |
| mailbox.add(message); // Add message to queue | |
| } |
| try { | ||
| Message message = mailbox.take(); // Wait for a message | ||
| onReceive(message); // Process it | ||
| } catch (InterruptedException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Improper handling of InterruptedException.
The catch block only resets the interrupt flag but doesn't properly handle actor termination, which could lead to thread leaks.
Current Code (Diff):
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ active = false; // Ensure actor terminates when interrupted
+ }📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| } catch (InterruptedException e) { | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| active = false; // Ensure actor terminates when interrupted |
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| public class ActorSystem { | ||
| private final ExecutorService executor = Executors.newCachedThreadPool(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚡ Performance Issue
Unbounded Thread Pool Risk.
Using an unbounded cached thread pool can lead to resource exhaustion under high load, potentially causing OutOfMemoryError.
Current Code (Diff):
- private final ExecutorService executor = Executors.newCachedThreadPool();
+ private final ExecutorService executor = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors());📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| private final ExecutorService executor = Executors.newCachedThreadPool(); | |
| private final ExecutorService executor = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors()); |
| * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| * THE SOFTWARE. | ||
| */ | ||
| package com.iluwatar.actor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Package name mismatch with imported classes.
The test class is in package 'com.iluwatar.actor' but imports classes from 'com.iluwatar.actormodel', which could cause class resolution issues.
Current Code (Diff):
- package com.iluwatar.actor;
+ package com.iluwatar.actormodel;📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| package com.iluwatar.actor; | |
| package com.iluwatar.actormodel; |
| // Wait briefly to allow async processing | ||
| Thread.sleep(200); | ||
|
|
||
| // Check that Srijan received the message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Incorrect assertion comment.
The comment states 'Check that Srijan received the message' but the assertion actually checks if Ansh received the message, which verifies the wrong behavior.
Current Code (Diff):
- // Check that Srijan received the message
+ // Check that Ansh received the message📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| // Check that Srijan received the message | |
| // Check that Ansh received the message |
| * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| * THE SOFTWARE. | ||
| */ | ||
| package com.iluwatar.table.inheritance; /* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Malformed package declaration with embedded comment.
The package declaration has an embedded multi-line comment which will cause a compilation failure as Java doesn't allow comments within package declarations.
Current Code (Diff):
- package com.iluwatar.table.inheritance; /*
+ package com.iluwatar.table.inheritance;
+ /*📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| package com.iluwatar.table.inheritance; /* | |
| package com.iluwatar.table.inheritance; |
| * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| * THE SOFTWARE. | ||
| */ | ||
| package com.iluwatar.table.inheritance; /* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Malformed package declaration with embedded comment.
The package declaration has a license comment embedded in the same line, which will cause compilation failure as package declarations must be standalone statements.
Current Code (Diff):
- package com.iluwatar.table.inheritance; /*
+ package com.iluwatar.table.inheritance;
+ /*📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
| package com.iluwatar.table.inheritance; /* | |
| package com.iluwatar.table.inheritance; |
PR Summary
Actor Model Pattern Implementation with Minor Refactoring
Overview
This PR introduces a new Actor Model design pattern implementation and includes various code refactoring improvements across multiple packages.
Change Types
Affected Modules
com.iluwatar.actormodel.*com.iluwatar.idempotentconsumer.*com.iluwatar.Moneycom.iluwater.money.MoneyTestcom.iluwatar.monolithic.*com.iluwatar.publish.subscribe.*com.iluwatar.servicestub.*com.iluwatar.sessionfacade.*com.iluwatar.table.inheritance.*Notes for Reviewers