You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "Jzjsnow (via GitHub)" <gi...@apache.org> on 2025/04/15 13:05:25 UTC
[PR] [AMORO-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Jzjsnow opened a new pull request, #3520:
URL: https://github.com/apache/amoro/pull/3520
<!--
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?
Currently the primary keys of the tables `table_optimizing_process`, `task_runtime`, and `optimizing_task_quota` contain only `process_id` and not `table_id`, where `process_id` is created by the current timestamp, e.g. 1744702084683. When multiple tables are optimized concurrently, it is easy to have the same `process_id` at the same trigger time, resulting in a primary key duplication exception `org.apache.ibatis.exceptions.PersistenceException` during persistence.
Close #3252.
## Brief change log
<!--
Clearly describe the changes made in modules, classes, methods, etc.
-->
- Add table_id to the primary keys of `table_optimizing_process`, `task_runtime`, and `optimizing_task_quota` respectively.
## 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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3520:
URL: https://github.com/apache/amoro/pull/3520#issuecomment-2943946970
## [Codecov](https://app.codecov.io/gh/apache/amoro/pull/3520?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 28.28%. Comparing base [(`e69710a`)](https://app.codecov.io/gh/apache/amoro/commit/e69710ae595652302ad4f49f3e1c38f989ed3280?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`e950979`)](https://app.codecov.io/gh/apache/amoro/commit/e95097969d6913daebb12920bc5b962cf47ee31e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 4 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3520 +/- ##
============================================
+ Coverage 21.76% 28.28% +6.51%
- Complexity 2391 3710 +1319
============================================
Files 436 617 +181
Lines 40498 49794 +9296
Branches 5743 6434 +691
============================================
+ Hits 8816 14083 +5267
- Misses 30935 34699 +3764
- Partials 747 1012 +265
```
| [Flag](https://app.codecov.io/gh/apache/amoro/pull/3520/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/3520/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `28.28% <100.00%> (?)` | |
| [trino](https://app.codecov.io/gh/apache/amoro/pull/3520/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/3520?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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "Jzjsnow (via GitHub)" <gi...@apache.org>.
Jzjsnow commented on PR #3520:
URL: https://github.com/apache/amoro/pull/3520#issuecomment-2871212139
> Hi, Thanks for driving this.
>
> After I listed all SQL related to `table_optimizing_process`, I found that the condition of some SQL does not contain `table_name`, which may cause poor performance, like: https://github.com/apache/amoro/blob/master/amoro-ams/src/main/java/org/apache/amoro/server/persistence/mapper/OptimizingMapper.java#L147
>
> I think the `table_optimizing_process` might only need a globally unique ID as the primary key, without necessarily including `table_name`. However, the current ID generation rules do indeed carry a risk of duplication.
>
> Currently, we use the`currentTimestamp` generation rule, which I understand is for easier cleanup operations. However, perhaps we should optimize the current cleanup logic, for example, by partitioning this table and using DROP PARTITION to perform cleanup more efficiently. #3445 is following the cleaning improvement issue.
@zhoujinsong Thanks for the viewpoint. I think expiring all tables during cleanup optimization can greatly improve optimization performance as discussed in #3445. On the other hand, I think it is still necessary to keep the `tableid` in the query sql related to tables `table_optimizing_process`/`task_runtime` / `optimizing_task_quota` because we may still need to process the optimization information of a single table.
For example, as mentioned in #3445, we'd better clean up all the optimization information of a table immediately when it is dropped, and in #3554 when we clear orphan table information during table service initializing, we still need tableid to filter out the optimizing entries related to the invalid tables.
--
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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on PR #3520:
URL: https://github.com/apache/amoro/pull/3520#issuecomment-2974994526
I am not sure if the new change can resolve the conflicts of process ID generation.
Different groups will generate processes at the same time, which may generate the same process ID.
HDYT?
--
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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong merged PR #3520:
URL: https://github.com/apache/amoro/pull/3520
--
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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "Jzjsnow (via GitHub)" <gi...@apache.org>.
Jzjsnow commented on PR #3520:
URL: https://github.com/apache/amoro/pull/3520#issuecomment-2890510403
> I think the `table_optimizing_process` might only need a globally unique ID as the primary key, without necessarily including `table_name`. However, the current ID generation rules do indeed carry a risk of duplication.
Thanks to the tip about the process_id unique key, I have since researched the use of the `table_optimizing_process` table in depth. I found that `process_id` is critical to apply as a primary key, such as canceling the optimizing process (from the front end) and updating the taskRuntime etc.
I agree that using timestamps as `process_id` is better for cleanup process performance, and I think the solution to the current problem of having duplicate timestamps is clearer and more concise: the duplicate `process_id` occurs due to the highly concurrent execution of the `TableOptimizingProcess#planInternal()`.
In the latest commit, i generate the `process_id` with a timestamp before the asynchronization to ensure the uniqueness of the `process_id`, PTAL.
--
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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #3520:
URL: https://github.com/apache/amoro/pull/3520#issuecomment-2943418364
LGTM for the new change, as the change only changes the `planTime` generate logic, and the `planTime`, which was generated in a single thread, the change here avoids the duplication of `planTime` for different tables before.
I'm not sure if there's a chance of duplication of `planTime` in the multi-node scenario, but that can be addressed in a separate issue if it exists.
--
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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "Jzjsnow (via GitHub)" <gi...@apache.org>.
Jzjsnow commented on PR #3520:
URL: https://github.com/apache/amoro/pull/3520#issuecomment-2939735053
> @Jzjsnow Hi,After resolving the conflict, we can continue with the review
Sure, I've resolved the conflict and rebased the changes. PTAL
--
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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on PR #3520:
URL: https://github.com/apache/amoro/pull/3520#issuecomment-2834439409
Hi, Thanks for driving this.
After I listed all SQL related to `table_optimizing_process`, I found that the condition of some SQL does not contain `table_name`, which may cost poor performance, like: https://github.com/apache/amoro/blob/master/amoro-ams/src/main/java/org/apache/amoro/server/persistence/mapper/OptimizingMapper.java#L147
I think the `table_optimizing_process` might only need a globally unique ID as the primary key, without necessarily including `table_name`. However, the current ID generation rules do indeed carry a risk of duplication.
Currently, we use the`currentTimestamp` generation rule, which I understand is for easier cleanup operations. However, perhaps we should optimize the current cleanup logic, for example, by partitioning this table and using DROP PARTITION to perform cleanup more efficiently.
#3445 is following the cleaning improvement issue.
--
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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "czy006 (via GitHub)" <gi...@apache.org>.
czy006 commented on PR #3520:
URL: https://github.com/apache/amoro/pull/3520#issuecomment-2939461524
@Jzjsnow Hi,After resolving the conflict, we can continue with the review
--
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-3252] Fix primary key duplicate exception when concurrently inserting table optimization process entries into database [amoro]
Posted by "Jzjsnow (via GitHub)" <gi...@apache.org>.
Jzjsnow commented on PR #3520:
URL: https://github.com/apache/amoro/pull/3520#issuecomment-3073529743
@xxubai @zhoujinsong Thanks a lot for the feedback!
We’ve made the change to use Snowflake IDs instead of timestamps. This should fix the uniqueness processId issue between different groups mentioned above, especially when processes are generated at the same time.
Hope this clears things up. Please take a look when you are free.
--
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