Skip to content
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

[ARORO-3289] Avoid calling getMixedTablePartitionSpecById in the scan loop #3290

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

7hong
Copy link
Contributor

@7hong 7hong commented Oct 22, 2024

Why are the changes needed?

Close #3289 .

How was this patch tested?

In my environment, there is a table with many partitions waiting to be merged. Each plan is very time-consuming. After optimization, the time-consuming is significantly reduced

image

@github-actions github-actions bot added the module:ams-server Ams server module label Oct 22, 2024
@7hong
Copy link
Contributor Author

7hong commented Oct 24, 2024

@zhoujinsong @majin1102 Do you have time to review it? Thanks

@baiyangtx
Copy link
Contributor

baiyangtx commented Nov 6, 2024

image

Will the old code trigger a reload of TableMetadata?

It seems that if the reloading of TableMetadata is not triggered, the performance of the new code is the same as that of the old code.

Iceberg TableMetadata also returns Specs Map objects from memory

image
image

@7hong
Copy link
Contributor Author

7hong commented Nov 7, 2024

@baiyangtx
Yes, the old code will refresh the TableMetadata. Especially when calling the getUGI method in an environment with Kerberos enabled, it will enter a synchronous blocking state.

trigger CI

Add getSpec to TableFileScanHelper
Copy link
Contributor

@czy006 czy006 left a comment

Choose a reason for hiding this comment

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

If there are Fire flame Graph or some unit test cases, it would be best.

@7hong
Copy link
Contributor Author

7hong commented Dec 2, 2024

@czy006
Flame graph before optimization:
image

Flame graph after optimization:

image

@Aireed Aireed mentioned this pull request Dec 4, 2024
33 tasks
Copy link
Contributor

@zhoujinsong zhoujinsong left a comment

Choose a reason for hiding this comment

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

Based on the information provided by the flame graph, Table#specs does not reload table metadata. From its internal implementation, this process is controlled by the shouldRefresh variable. However, this call does trigger the Kerberos relogin check logic, which is unnecessary.

So, I am +1 with this improvement.

@zhoujinsong zhoujinsong merged commit 530b700 into apache:master Dec 6, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement]: Avoid calling getMixedTablePartitionSpecById in the loop
4 participants