You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "wardlican (via GitHub)" <gi...@apache.org> on 2025/09/04 09:10:41 UTC
[PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
wardlican opened a new pull request, #3764:
URL: https://github.com/apache/amoro/pull/3764
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://amoro.apache.org/how-to-contribute/
2. If the PR is related to an issue in https://github.com/apache/amoro/issues, add '[AMORO-XXXX]' in your PR title, e.g., '[AMORO-XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][AMORO-XXXX] Your PR title ...'.
-->
## Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about its use case.
2. If you fix a bug, you can clarify why it is a bug.
3. Use Fix/Resolve/Close #{ISSUE_NUMBER} to link this PR to its related issue
-->
Close #xxx.
## Brief change log
Solution Overview
Added a new SPI factory interface, TableServiceProvider (based on ServiceLoader).
Provides a default implementation, DefaultTableServiceProvider, and registers it in META-INF/services.
Added a new configuration option, TABLE_SERVICE_IMPL (string), with two supported syntaxes:
-- Specify the provider name (e.g., default)
-- Specify the implementation class name (e.g., org.apache.amoro.server.table.DefaultTableService)
Creates a TableService in the AmoroServiceContainer using a configuration-based loader method.
Retains the existing default behavior: Falling back to DefaultTableService when not configured.
-
## How was this patch tested?
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] Run test locally before making a pull request
## Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "baiyangtx (via GitHub)" <gi...@apache.org>.
baiyangtx commented on PR #3764:
URL: https://github.com/apache/amoro/pull/3764#issuecomment-3264351244
> To distinguish between the master-slave mode and the master-slave mode, the newly added master-slave mode should not affect the original logic. The master-slave mode will make more logical changes in TableServcie.
Will the two implementations be merged in the future?
In the long run, the horizontally-scalable version is likely to become the default. If we do expect a single unified implementation eventually, we could simply switch between the two code paths with a config flag today instead of introducing a full SPI framework. After all, TableService is not intended to be opened up for third-party plug-ins down the road.
WDYT @zhoujinsong @Aireed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3764:
URL: https://github.com/apache/amoro/pull/3764#issuecomment-3263816315
## [Codecov](https://app.codecov.io/gh/apache/amoro/pull/3764?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 21.71%. Comparing base ([`e3c16af`](https://app.codecov.io/gh/apache/amoro/commit/e3c16af3d382a708c6e18e70589f3d4ee575de49?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e4e5f3b`](https://app.codecov.io/gh/apache/amoro/commit/e4e5f3bfc792a77d7c4e6d652dadcecd8c18c36e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (e3c16af) and HEAD (e4e5f3b). Click for more details.
>
> <details><summary>HEAD has 3 uploads less than BASE</summary>
>
>| Flag | BASE (e3c16af) | HEAD (e4e5f3b) |
>|------|------|------|
>|core|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3764 +/- ##
============================================
- Coverage 28.53% 21.71% -6.82%
+ Complexity 3759 2388 -1371
============================================
Files 617 439 -178
Lines 50056 40611 -9445
Branches 6482 5743 -739
============================================
- Hits 14283 8819 -5464
+ Misses 34760 31047 -3713
+ Partials 1013 745 -268
```
| [Flag](https://app.codecov.io/gh/apache/amoro/pull/3764/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [core](https://app.codecov.io/gh/apache/amoro/pull/3764/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [trino](https://app.codecov.io/gh/apache/amoro/pull/3764/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `21.71% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/amoro/pull/3764?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3764:
URL: https://github.com/apache/amoro/pull/3764#issuecomment-3434601331
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "baiyangtx (via GitHub)" <gi...@apache.org>.
baiyangtx commented on code in PR #3764:
URL: https://github.com/apache/amoro/pull/3764#discussion_r2327378787
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/TableService.java:
##########
@@ -46,4 +47,16 @@ default boolean contains(Long tableId) {
* @return managed table.
*/
AmoroTable<?> loadTable(ServerTableIdentifier identifier);
+
+ /** Explore and synchronize table runtimes from catalogs. Intended for periodic sync and tests. */
+ void exploreTableRuntimes();
Review Comment:
Who will call those methods?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on PR #3764:
URL: https://github.com/apache/amoro/pull/3764#issuecomment-3284193114
> > To distinguish between the master-slave mode and the master-slave mode, the newly added master-slave mode should not affect the original logic. The master-slave mode will make more logical changes in TableServcie.
>
> Will the two implementations be merged in the future? In the long run, the horizontally-scalable version is likely to become the default. If we do expect a single unified implementation eventually, we could simply switch between the two code paths with a config flag today instead of introducing a full SPI framework. After all, TableService is not intended to be opened up for third-party plug-ins down the road.
>
> WDYT @zhoujinsong @Aireed
Yes, the design here needs to decide whether to implement compatible logic through configuration in DefaultTableService.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3764:
URL: https://github.com/apache/amoro/pull/3764#issuecomment-3408682091
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@amoro.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on code in PR #3764:
URL: https://github.com/apache/amoro/pull/3764#discussion_r2328978202
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/TableService.java:
##########
@@ -46,4 +47,16 @@ default boolean contains(Long tableId) {
* @return managed table.
*/
AmoroTable<?> loadTable(ServerTableIdentifier identifier);
+
+ /** Explore and synchronize table runtimes from catalogs. Intended for periodic sync and tests. */
+ void exploreTableRuntimes();
Review Comment:
This method is mentioned in the interface for compatibility with the original test cases in SPI mode, and secondly, this method also needs to be implemented in master-slave mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on PR #3764:
URL: https://github.com/apache/amoro/pull/3764#issuecomment-3264321760
> t TableService? Whether to deploy in multi-node mode should be controlled by a simple configuration flag; it shouldn’t require a completely separate TableService implementation.
To distinguish between the master-slave mode and the master-slave mode, the newly added master-slave mode should not affect the original logic. The master-slave mode will make more logical changes in TableServcie.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3764: [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods.
URL: https://github.com/apache/amoro/pull/3764
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on code in PR #3764:
URL: https://github.com/apache/amoro/pull/3764#discussion_r2328978202
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/TableService.java:
##########
@@ -46,4 +47,16 @@ default boolean contains(Long tableId) {
* @return managed table.
*/
AmoroTable<?> loadTable(ServerTableIdentifier identifier);
+
+ /** Explore and synchronize table runtimes from catalogs. Intended for periodic sync and tests. */
+ void exploreTableRuntimes();
Review Comment:
This method is mentioned in the interface for compatibility with the original test cases in SPI mode, and secondly, this method also needs to be implemented in master-slave mode. @baiyangtx @zhoujinsong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "wardlican (via GitHub)" <gi...@apache.org>.
wardlican commented on PR #3764:
URL: https://github.com/apache/amoro/pull/3764#issuecomment-3290830285
> I'm also hesitant about whether it's necessary to add SPI extension. Perhaps it would be better if the existing implementation simply supports horizontal scaling, as the master-slave architecture will eventually be replaced by a multiple-master architecture.
OK, here i will change it to implement the master-slave mode logic in DefaultTableService through configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Amoro-3762][Subtask] Change TableService to SPI mode to support expansion of other implementation methods. [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on PR #3764:
URL: https://github.com/apache/amoro/pull/3764#issuecomment-3284208048
I'm also hesitant about whether it's necessary to add SPI extension. Perhaps it would be better if the existing implementation simply supports horizontal scaling, as the master-slave architecture will eventually be replaced by a multiple-master architecture.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org