feat: create builder for disk manager #16191
Conversation
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
|
Correct me if I'm wrong, but the failing test doesn't seem related. |
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
alamb
left a comment
There was a problem hiding this comment.
Thank you @jdrouet -- looks great to me
I do think we should deprecate DiskManagerConfig -- as described on https://datafusion.apache.org/contributor-guide/api-health.html if possible, but we can do that as a follow on PR
2010YOUY01
left a comment
There was a problem hiding this comment.
LGTM, thank you. I also think it's great to deprecating the old APIs.
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
| "Created local dirs {local_dirs:?} as DataFusion working directory" | ||
| ); | ||
| Ok(DiskManager { | ||
| local_dirs: Mutex::new(Some(local_dirs)), |
There was a problem hiding this comment.
Thank you @jdrouet for the work, LGTM, and minor question, do we have each dir max limit config when we have multi dirs?
There was a problem hiding this comment.
Actually, I just moved the existing to a builder. So right now, we have the same behavior as before, meaning that each dir has the same limit.
For more details, you'd have to go back to the current_file_disk_usage computation.
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
xudong963
left a comment
There was a problem hiding this comment.
Thank you, the change will match our codebase more, most of places are using Builder style
Which issue does this PR close?
DiskManagerBuilderto construct DiskManagers #15319Rationale for this change
Proposing a builder pattern for the disk managers.
What changes are included in this PR?
In order not to have a breaking change, I decided not to update the current config structure, but instead, create a builder that would be an alternative and would allow to customize the disk manager, while being easily extendible.
Are these changes tested?
It's been used in the existing tests and the CLI
Are there any user-facing changes?
Introducing a builder structure.