Add max_file_download_size config and expose to DataSource #2080
Conversation
| class Builder: | ||
| def __init__(self): | ||
| self.max_file_size = DEFAULT_MAX_FILE_SIZE | ||
|
|
||
| def with_max_file_size(self, max_file_size): | ||
| self.max_file_size = max_file_size | ||
| return self | ||
|
|
||
| def build(self): | ||
| return DataSourceFrameworkConfig(self.max_file_size) |
There was a problem hiding this comment.
Curious - why use builder here?
For now it's only one property, and probably in future it'll be like 10.
Do you see partial initialisation of this config (e.g. with some properties initialised, some not), or multistep initialisation of config?
There was a problem hiding this comment.
I want to make it easy for this to be formed up from multiple places eventually. We had a recent slack thread where we discussed how some service configs might one day have a UI and be stored in ES. And we've got some in the YAML, and maybe would someday have some in RCFs. Passing around a half-complete builder is a good pattern to use for scenarios like this where multiple actors are all contributing parts to something being built.
There was a problem hiding this comment.
Yeah I understand that, though I felt that purpose of Builder is often to have a mutable object until at some point you're ready to create an immutable object and use it somewhere. Since Python has different encapsulation concepts, it's much less necessary, and Builder becomes more of a "convenience" class that makes creation of objects more pleasant.
I'd think that just having DataSourceFrameworkConfig class with getters/setters should be fine.
This however should not prevent from merging change as is, it's rather a discussion to see your POV here :)
There was a problem hiding this comment.
I think the benefit of Builder pattern is that it makes the object instantiation simple and readable when there are a lot of variables. Apparently this is implemented for future-proof.
Do you think we should define the constructor of DataSourceFrameworkConfig like this:
def __init__(self, builder):
self.max_file_size = builder.max_file_sizeand the build method like this:
def build(self):
return DataSourceFrameworkConfig(self)There was a problem hiding this comment.
I think the truth is there will be N sources that provide config options, e.g.:
- Configuration file
- Connector-specific config
- Deployment-wide settings
So builder will have 3 args/3 with_x methods and number is unlikely to grow IMO, I might be wrong here.
As to the syntax, I think original syntax in Sean's changeset is already good Builder syntax - only improvement (also absolutely unnecessary) could be is to make DataSourceFrameworkConfig have read-only properties with data stored in __-prefixed keys so that they could not be accessed, but I don't think it matters a lot.
There was a problem hiding this comment.
I think the truth is there will be N sources that provide config options, e.g.:
- Configuration file
- Connector-specific config
- Deployment-wide settings
So builder will have 3 args/3 with_x methods
I think the builder should not care of where this config is coming from. Take max_file_size as example, it should only define one method with_max_file_size. If it's from configuration file, you can do builder.with_max_file_size(loaded_config.max_file_size), and if it's from a connector, you can do builder.with_max_file_size(connector.max_file_size)
There was a problem hiding this comment.
Let's just keep these options in mind as this grows. I think we've agreed that the current state is "fine", and that it could be improved on depending on how we use it in the future. So let's just plan to refactor as necessary as its usage grows.
| if attachment_size > self.framework_config.max_file_size: | ||
| self._logger.warning( | ||
| f"File size {attachment_size} of file {attachment_name} is larger than {FILE_SIZE_LIMIT} bytes. Discarding file content" | ||
| f"File size {attachment_size} of file {attachment_name} is larger than {self.framework_config.max_file_size} bytes. Discarding file content" |
There was a problem hiding this comment.
I was also wondering if it's possible to move this condition elsewhere, so that each connector would not call it?
Best I could think of was that rather than doing comparison manually, make the class "richer" and do something like:
if self.framework_config.file_size_supported(attachment_size):
# download
How do you feel?
There was a problem hiding this comment.
I'm not going to tackle that as part of this - my focus for this PR is making the existing logic configurable. I believe there was debate previously about moving this kind of thing to the Extraction utils, but that ended up being rejected, and I don't want to re-open that discussion at the moment.
💔 Failed to create backport PR(s)The backport operation could not be completed due to the following error: The backport PRs will be merged automatically after passing CI. To backport manually run: |
Closes https://github.com/elastic/enterprise-search-team/issues/4716
Adds a new config,
service.max_file_download_sizethat defaults to 10MB.Also, adds a new abstraction for specific framework configs that we want to expose to the DataSource instances, without exposing all of our YAML configs. This PR exposes only the new
max_file_size, but can be easily updated to expose more (follow-up work: https://github.com/elastic/enterprise-search-team/issues/5057).Checklists
Pre-Review Checklist
v7.13.2,v7.14.0,v8.0.0)Release Note
service.max_file_download_sizecan now be set in theconfig.ymlto control how large of files the framework will attempt to download. This config is only used for connectors that are not using the Data Extraction Service.