Skip to content

Conversation

@komamitsu
Copy link
Contributor

@komamitsu komamitsu commented Jun 12, 2025

Description

We supported replicationTables arguments in SchemaLoader's load(), unload() and repairAll() in #2747. The methods are public and it's possible existing users has used them. So, this PR adds the original method signatures for backward compatibility.

Related issues and/or PRs

#2747

Changes made

Added the original method signatures that call corresponding the new method.

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)

None

Release notes

N/A

}
}

// For backward compatibility with the old method signatures.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied these test cases from the original test code right before #2747 was merged.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR restores the original SchemaLoader method signatures for load(), unload(), and repairAll() to maintain backward compatibility by forwarding calls to the newer overloads with a default false flag.

  • Introduces public and private overloads without the replicationTables parameter.
  • Each added overload delegates to the corresponding new method with a false value for the extra flag.
  • Ensures existing client code using the old signatures continues to compile and run.
Comments suppressed due to low confidence (2)

schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaLoader.java:640

  • [nitpick] The parameter name repairCoordinatorTable is singular but the other methods use plural (deleteCoordinatorTables/createCoordinatorTables); consider renaming to repairCoordinatorTables for consistency.
public static void repairAll(

schema-loader/src/main/java/com/scalar/db/schemaloader/SchemaLoader.java:456

  • Add unit tests covering these new overloads to verify they correctly delegate to the new methods with default false flags.
// For backward compatibility.

* @param options specific options for creating tables.
* @param createCoordinatorTables create coordinator tables or not.
* @throws SchemaLoaderException thrown when creating tables fails.
*/
Copy link

Copilot AI Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These backward-compatibility overloads should be annotated with @deprecated to signal that they will be removed in a future release and guide users towards the new signatures.

Suggested change
*/
*/
@Deprecated

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments. Other than that, LGTM! Thank you!

}
}

// For backward compatibility.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we don’t need this comment if we’re not planning to remove these methods in a future release. Or do we plan to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed it in e7b6082

}
}

// For backward compatibility with the old method signatures.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed it in e7b6082

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@komamitsu komamitsu merged commit 30057d8 into master Jun 13, 2025
55 checks passed
@komamitsu komamitsu deleted the backward-compatible-for-schema-loader branch June 13, 2025 06:11
feeblefakie pushed a commit that referenced this pull request Jun 13, 2025
…d `repairAll()` for backward compatibility (#2760)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants