Refactor ParallelExecutor #2820

brfrn169 · 2025-06-26T04:54:17Z

Description

This PR refactors ParallelExecutor.

Related issues and/or PRs

N/A

Changes made

Added some inline comments. Please take a look for the details.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

N/A

gemini-code-assist

Summary of Changes

Hello @brfrn169, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request primarily focuses on enhancing the robustness and maintainability of the ParallelExecutor class by refactoring its core task execution and exception handling mechanisms. It introduces a more structured approach to managing exceptions, particularly when multiple failures can occur, and includes a minor performance optimization for single-task operations. The changes are backed by a substantial increase in unit test coverage.

Highlights

Exception Handling Refinement: I've significantly refactored the exception handling logic within executeTasksInParallel and executeTasksSerially. This includes centralizing the rethrowing of specific exception types into a new rethrow helper method, and improving how multiple exceptions are aggregated using Throwable.addSuppressed() when stopOnError is false, ensuring all relevant failures are captured.
Single Task Optimization: For scenarios where only a single task needs to be executed and noWait is false, I've optimized the executeTasks method to directly run the task, bypassing the overhead of parallel execution setup.
Testability and Coverage: The executeTasks method's visibility has been adjusted to package-private and annotated with @VisibleForTesting to facilitate more comprehensive unit testing. I've also added extensive new tests for executeTasks to cover various parallel, serial, single-task, and exception handling scenarios, including the new suppressed exception behavior.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

Copilot

Pull Request Overview

This PR refactors ParallelExecutor by simplifying task execution paths, unifying exception handling via a new rethrow helper, and exposing executeTasks for testing. It also adds comprehensive unit tests for various execution and error scenarios.

Changed executeTasks visibility and inlined single-task handling.
Rewrote parallel execution loop and exception aggregation.
Introduced rethrow method and cleaned up serial execution logic.
Added new tests covering parallel/serial, noWait, and stopOnError combinations.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
core/src/main/java/com/scalar/db/transaction/consensuscommit/ParallelExecutor.java	Refactored `executeTasks`, rewrote loops, added `rethrow` and removed duplicate branching.
core/src/test/java/com/scalar/db/transaction/consensuscommit/ParallelExecutorTest.java	Added many new test cases and a `mock` import; missing some static imports.

Comments suppressed due to low confidence (1)

core/src/test/java/com/scalar/db/transaction/consensuscommit/ParallelExecutorTest.java:8

The test uses when(...) and times(...) but does not import these statics. Please add import static org.mockito.Mockito.when; and import static org.mockito.Mockito.times; to avoid compilation errors.

import static org.mockito.Mockito.mock;

Copilot · 2025-06-26T04:55:12Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ParallelExecutor.java

    for (ParallelExecutorTask task : tasks) {
      try {
        task.run();
      } catch (ExecutionException | ValidationConflictException | CrudException e) {


[nitpick] Serial execution no longer logs failures while parallel execution does. Consider re-adding a logger.warn inside this catch so serial runs also record taskName and transactionId on error.

gemini-code-assist

Code Review

This PR refactors the ParallelExecutor to simplify exception handling and improve code structure. The introduction of a rethrow helper method and the use of suppressed exceptions are great improvements. However, the refactoring has removed critical exception logging for serial execution paths, which could hinder debugging.

gemini-code-assist · 2025-06-26T04:56:48Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ParallelExecutor.java


-  private void executeTasksSerially(
-      List<ParallelExecutorTask> tasks, boolean stopOnError, String taskName, String transactionId)
+  private void executeTasksSerially(List<ParallelExecutorTask> tasks, boolean stopOnError)


The refactoring removed the logging of exceptions that occur during serial task execution. The logger.warn call was present in the previous version and provided valuable context, which is crucial for debugging. Consider adding the logging back to provide better context when exceptions occur.

private void executeTasksSerially(List<ParallelExecutorTask> tasks, boolean stopOnError, String taskName, String transactionId) throws ExecutionException, ValidationConflictException, CrudException { Exception exception = null; for (ParallelExecutorTask task : tasks) { try { task.run(); } catch (ExecutionException | ValidationConflictException | CrudException e) { logger.warn("Failed to run a {} task. Transaction ID: {}", taskName, transactionId, e); if (!stopOnError) { if (exception == null) { exception = e; } else { exception.addSuppressed(e); } } else { throw e; } } } if (!stopOnError && exception != null) { rethrow(exception); } }

brfrn169 · 2025-06-26T04:54:57Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ParallelExecutor.java

      // If there is only one task and noWait is false, we can run it directly without parallel
      // execution.
-      executeTasksSerially(tasks, stopOnError, taskName, transactionId);
+      tasks.get(0).run();


Change to directly call tasks.get(0).run().

brfrn169 · 2025-06-26T04:55:55Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ParallelExecutor.java

+            throwable = cause;
          } else {
-            throw new AssertionError("Can't reach here. Maybe a bug", e);
+            throwable.addSuppressed(cause);


Updated to use addSuppressed().

brfrn169 · 2025-06-26T04:56:17Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ParallelExecutor.java

+          if (exception == null) {
+            exception = e;
+          } else {
+            exception.addSuppressed(e);


Ditto. Updated to use addSuppressed().

brfrn169 · 2025-06-26T05:15:33Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ParallelExecutor.java

      try {
        task.run();
      } catch (ExecutionException | ValidationConflictException | CrudException e) {
-        logger.warn("Failed to run a {} task. Transaction ID: {}", taskName, transactionId, e);


After using addSuppressed(), we can remove this log because all exceptions should now be included as suppressed exceptions.

brfrn169 · 2025-06-26T05:18:13Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ParallelExecutor.java

+              logger.warn(
+                  "Failed to run a {} task. Transaction ID: {}", taskName, transactionId, e);


We need to keep this log because, even after using addSuppressed(), some exceptions may not be included as suppressed when stopOnError is set to true.

komamitsu

LGTM! 👍

feeblefakie

LGTM! Thank you!
BTW, I'm just wondering what the motivation is for the refactoring?
It would be great if you could clarify it.

Torch3333

LGTM, thank you!

brfrn169 · 2025-07-02T08:31:06Z

BTW, I'm just wondering what the motivation is for the refactoring?
It would be great if you could clarify it.

@feeblefakie The initial motivation was to use the Throwable.addSuppressed() method in ParallelExecutor, as multiple exceptions can be thrown during parallel execution. However, while working on that, I ended up refactoring some other parts of the logic as well. Thanks.

Refactor ParallelExecutor

88ac071

brfrn169 requested a review from Copilot June 26, 2025 04:54

brfrn169 self-assigned this Jun 26, 2025

brfrn169 added the improvement label Jun 26, 2025

gemini-code-assist bot reviewed Jun 26, 2025

View reviewed changes

Copilot AI reviewed Jun 26, 2025

View reviewed changes

gemini-code-assist bot reviewed Jun 26, 2025

View reviewed changes

brfrn169 commented Jun 26, 2025

View reviewed changes

brfrn169 requested review from Torch3333, feeblefakie and komamitsu June 26, 2025 05:19

komamitsu approved these changes Jun 27, 2025

View reviewed changes

feeblefakie approved these changes Jul 2, 2025

View reviewed changes

Torch3333 approved these changes Jul 2, 2025

View reviewed changes

Merge branch 'master' into refactor-ParallelExecutor

dda7ab6

brfrn169 merged commit 36f4142 into master Jul 2, 2025
56 checks passed

brfrn169 deleted the refactor-ParallelExecutor branch July 2, 2025 10:50

feeblefakie pushed a commit that referenced this pull request Jul 2, 2025

Refactor ParallelExecutor (#2820)

659f856

feeblefakie mentioned this pull request Jul 2, 2025

Backport to branch(3) : Refactor ParallelExecutor #2824

Merged

		logger.warn(
		"Failed to run a {} task. Transaction ID: {}", taskName, transactionId, e);

Refactor ParallelExecutor #2820

Refactor ParallelExecutor #2820

Uh oh!

Conversation

brfrn169 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

Torch3333 left a comment

Choose a reason for hiding this comment

Uh oh!

brfrn169 commented Jul 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brfrn169 commented Jun 26, 2025 •

edited

Loading