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

arrow/parquet : limit the parquet-reader memory usage #163

Merged
merged 1 commit into from
Oct 6, 2024

Conversation

galsalomon66
Copy link

@galsalomon66 galsalomon66 commented Aug 27, 2024

using reader-properties to set buffer size upon reading column chunks.
it is useful for very big row-groups(huge number of values per column).

https://bugzilla.redhat.com/show_bug.cgi?id=2252403

@@ -747,6 +747,15 @@ class PARQUET_EXPORT RowGroupReader {
std::unique_ptr<Contents> contents_;
};

//TODO external setting? RGW options ??
#define RGW_buffer_size 1024*1024*16
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you pass it as a parameter to the function that init the parquet reader?
should probably be an RGW conf parameter passed from the outside

Copy link
Author

Choose a reason for hiding this comment

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

@yuvalif
sure.
that is the plan. (review the comment above)

i measured whether it resolved the issue of huge memory consumption.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. but we should probably merge after we make the change to take the size as a parameter, so we can integrate into the rgw code

Copy link
Author

@galsalomon66 galsalomon66 Aug 29, 2024

Choose a reason for hiding this comment

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

yes, the interface (RGW option) is included in this PR.

with the standalone-application the impact of this change is visible(memory consumption, RSS).
it is also possible to measure other aspects of these changes, like the number of calls to storage systems.

this parameter probably has more impact upon using the S3 storage, we should strive for a default size that is optimized with RSS and throughput.
(users should not "play" with this parameter too much, this may cause other issues.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

how can an RGW option be included in an s3-select PR?
I thought that this PR would expose an interface where this value could be set, and the matching RGW PR (ceph/ceph#59465) will add a new option in: src/common/options/rgw.yaml.in and pass it to that API

Copy link
Author

Choose a reason for hiding this comment

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

yes.
i meant the Ref PR ceph/ceph#59465

@yuvalif
Copy link
Collaborator

yuvalif commented Aug 27, 2024

what would be the behavior when the max buffer size is exceeded?

@galsalomon66
Copy link
Author

galsalomon66 commented Aug 28, 2024

what would be the behavior when the max buffer size is exceeded?

the current behavior ... no limitation on buffer size.
that issue was discovered upon processing a large parquet object with a few row-groups(that is a bad parquet object)
it caused OOM for a low-end server, see the attached BZ.

with the current fix(the buffer size limitation) it loads part after part according to the buffer size.
this will impact throughput.
thus, the default setting should be quite high.

the arrow library does not raise an exception.

@yuvalif yuvalif self-requested a review September 5, 2024 07:50
@yuvalif
Copy link
Collaborator

yuvalif commented Sep 9, 2024

@galsalomon66 is 7ef7e67 related to the change?
if not, can you please squash: 9b9f357, a8cafe8 and 7ef7e67
to one commit ?

@galsalomon66
Copy link
Author

@galsalomon66 is 7ef7e67 related to the change? if not, can you please squash: 9b9f357, a8cafe8 and 7ef7e67 to one commit ?

the remove debug related to this PR, i used that for monitoring different settings.

@yuvalif
Copy link
Collaborator

yuvalif commented Sep 9, 2024

@galsalomon66 is 7ef7e67 related to the change? if not, can you please squash: 9b9f357, a8cafe8 and 7ef7e67 to one commit ?

the remove debug related to this PR, i used that for monitoring different settings.

ok. what about: ac758c1 ?

@galsalomon66
Copy link
Author

@galsalomon66 is 7ef7e67 related to the change? if not, can you please squash: 9b9f357, a8cafe8 and 7ef7e67 to one commit ?

the remove debug related to this PR, i used that for monitoring different settings.

ok. what about: ac758c1 ?

while testing the arrow/parquet change(buffer size limitation)
I found that the standalone application has the wrong setting upon parquet flow.
the wrong setting relates to response size (it could be quite big, depending on the type of query/object).

the standalone application has no impact on RGW.

I refactored the code referring to the buffer response size.

it is useful upon very row-groups(huge number of values per column).
refactor of the send-back-result-response.
add external configuration per parquet read-buffer. fix for result printing
remove debug.

Signed-off-by: Gal Salomon <[email protected]>
@galsalomon66 galsalomon66 force-pushed the limit_memory_usage_per_big_row_groups branch from 7ef7e67 to 4b126a7 Compare September 10, 2024 13:34
@galsalomon66 galsalomon66 merged commit 0a0f6d4 into master Oct 6, 2024
2 checks passed
@ktdreyer ktdreyer deleted the limit_memory_usage_per_big_row_groups branch December 12, 2024 17:03
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.

2 participants