-
Notifications
You must be signed in to change notification settings - Fork 277
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
[AMORO-2893] optimize table page loading #2914
[AMORO-2893] optimize table page loading #2914
Conversation
e5b1c32
to
cb8338a
Compare
Currently, this is a draft version just a better discussion. the open questions are
I'm leaning towards option 2.2 with not returning the table format to the front end because it reduces the number of DB requests by one Please let me what you think about this, thanks. @zhoujinsong @majin1102 |
Now that you have queried the TableRuntimeBean object from the database, why not directly perform sorting or paging operations on this object later? |
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.
Thanks for this improvement, I left some comments.
PTAL
...oro-ams-server/src/main/java/org/apache/amoro/server/persistence/mapper/TableMetaMapper.java
Outdated
Show resolved
Hide resolved
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.
Thanks for raising this improvement, I think it's quite necessary and left some comments
Please take a look
...s-server/src/main/java/org/apache/amoro/server/dashboard/controller/OptimizerController.java
Outdated
Show resolved
Hide resolved
...s-server/src/main/java/org/apache/amoro/server/dashboard/controller/OptimizerController.java
Outdated
Show resolved
Hide resolved
...amoro-ams-server/src/main/java/org/apache/amoro/server/dashboard/model/TableRuntimeBean.java
Outdated
Show resolved
Hide resolved
I think decoupling from TableService and TableManager is a good idea. |
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.
@majin1102 thanks for the review, I've replied all the comments, could you please take another look when you're free.
...s-server/src/main/java/org/apache/amoro/server/dashboard/controller/OptimizerController.java
Outdated
Show resolved
Hide resolved
...s-server/src/main/java/org/apache/amoro/server/dashboard/controller/OptimizerController.java
Outdated
Show resolved
Hide resolved
...oro-ams-server/src/main/java/org/apache/amoro/server/persistence/mapper/TableMetaMapper.java
Outdated
Show resolved
Hide resolved
...amoro-ams-server/src/main/java/org/apache/amoro/server/dashboard/model/TableRuntimeBean.java
Outdated
Show resolved
Hide resolved
@baiyangtx thanks for the review, the bottle-neck here is not the cost of the sort, but we retrieved too much data from DB, the solution here wants to limit the row number when retrieved from DB(the sort in SQL is needed because we want to show the 'running' status before the 'idle' status in the frontend) |
amoro-ams/amoro-ams-server/src/main/java/org/apache/amoro/server/table/TableService.java
Outdated
Show resolved
Hide resolved
@majin1102 sorry for the late reply, I've drafted a version such that: TableRuntimeMeta is used for retrieving from db, |
c0045d9
to
0a0f134
Compare
0a0f134
to
6d41230
Compare
acf96fe
to
956f91d
Compare
...oro-ams-server/src/main/java/org/apache/amoro/server/persistence/mapper/TableMetaMapper.java
Outdated
Show resolved
Hide resolved
956f91d
to
779fc06
Compare
779fc06
to
5750563
Compare
5750563
to
f8cacc1
Compare
@majin1102 @zhoujinsong @baiyangtx I've updated a version, could you please take a look at this when you're free, thanks |
60c5db0
to
016bd01
Compare
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.
Thanks for this improvement, I left some comments.
PTAL
amoro-ams/amoro-ams-server/src/main/resources/mysql/upgrade-0.7.0-to-0.7.1.sql
Outdated
Show resolved
Hide resolved
amoro-ams/amoro-ams-server/src/main/resources/mysql/upgrade-0.7.0-to-0.7.1.sql
Outdated
Show resolved
Hide resolved
...-ams/amoro-ams-server/src/main/java/org/apache/amoro/server/optimizing/OptimizingStatus.java
Outdated
Show resolved
Hide resolved
...oro-ams-server/src/main/java/org/apache/amoro/server/persistence/mapper/TableMetaMapper.java
Outdated
Show resolved
Hide resolved
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.
One more point,
the mysql/ams-mysql-init.sql
and postgres/ams-postgres-init.sql
files also need to be modified synchronously for the schema change.
@baiyangtx thanks very much for the review, will update the PR % the search pattern about the db soon, that change will change after we reach the consensus. |
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.
Thanks for the contribution!
I left some comments, you may want to checkout.
...s-server/src/main/java/org/apache/amoro/server/dashboard/controller/OptimizerController.java
Outdated
Show resolved
Hide resolved
...s-server/src/main/java/org/apache/amoro/server/dashboard/controller/OptimizerController.java
Outdated
Show resolved
Hide resolved
amoro-ams/amoro-ams-server/src/main/resources/mysql/ams-mysql-init.sql
Outdated
Show resolved
Hide resolved
...oro-ams-server/src/main/java/org/apache/amoro/server/persistence/mapper/TableMetaMapper.java
Outdated
Show resolved
Hide resolved
...s-server/src/main/java/org/apache/amoro/server/dashboard/controller/OptimizerController.java
Outdated
Show resolved
Hide resolved
amoro-ams/amoro-ams-server/src/main/java/org/apache/amoro/server/table/RuntimeHandlerChain.java
Outdated
Show resolved
Hide resolved
amoro-ams/amoro-ams-server/src/main/java/org/apache/amoro/server/table/RuntimeHandlerChain.java
Outdated
Show resolved
Hide resolved
amoro-ams/amoro-ams-server/src/main/java/org/apache/amoro/server/table/TableRuntime.java
Outdated
Show resolved
Hide resolved
amoro-ams/amoro-ams-server/src/main/java/org/apache/amoro/server/table/TableRuntime.java
Outdated
Show resolved
Hide resolved
...amoro-ams-server/src/main/java/org/apache/amoro/server/table/executor/BaseTableExecutor.java
Outdated
Show resolved
Hide resolved
@zhoujinsong thanks very much for the review, will update it and attach some result before and after apply the change |
3cb3d16
to
22c482b
Compare
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. |
sorry for the late reply, will rebase the latest master and attach the result asap. |
392fd22
to
bb4475b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2914 +/- ##
============================================
- Coverage 29.43% 22.42% -7.01%
+ Complexity 3685 2280 -1405
============================================
Files 568 379 -189
Lines 47267 38044 -9223
Branches 6196 5439 -757
============================================
- Hits 13912 8532 -5380
+ Misses 32372 28779 -3593
+ Partials 983 733 -250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
65e9316
to
5cc731f
Compare
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.
LGTM.
Thanks for the improvement! @klion26
be016ae
to
62f1bdd
Compare
tableId is unique for every table we can simplify the key using tableId, so that we don't need pass other parameters(maybe need retrieve from db) when retrieve info from cache
62f1bdd
to
fc434c8
Compare
…tables Befor the change we'll retrieve all tables from db and sort in the memory, this will be slow if there are many entriesin the db, after this change we sort in the db and only retrieve the needed entries.
fc434c8
to
a75639f
Compare
Sorry for being late. |
@zhoujinsong @majin1102 thanks for the review and merging! |
Why are the changes needed?
Close #2893 .
Optimize the loading process when user open the
optimizer
pageBrief change log
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