-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat(Prestissimo): Make Broadcast writes directories configurable #26671
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
Conversation
Summary: Adds support for configuring the directory used for Storage-based Broadcasts in Velox. The "config" is a transparent string, which is expected to be interpreted by the Filesystem. Differential Revision: D87552571
Reviewer's GuideIntroduces a new configuration property to customize directory creation for Storage-based Broadcasts in Velox by passing a user-defined string to the filesystem’s mkdir operation. Sequence diagram for BroadcastWriteOperator directory creation with configurable optionssequenceDiagram
participant "BroadcastWriteOperator"
participant "SystemConfig"
participant "FileSystem"
"BroadcastWriteOperator"->>"SystemConfig": broadcasterDirectoryCreateConfig()
"SystemConfig"-->>"BroadcastWriteOperator": config string
"BroadcastWriteOperator"->>"FileSystem": mkdir(basePath, dirOptions)
"FileSystem"-->>"BroadcastWriteOperator": Directory created
Class diagram for updated SystemConfig with broadcaster directory configclassDiagram
class SystemConfig {
+std::string spillerDirectoryCreateConfig() const
+std::string broadcasterDirectoryCreateConfig() const
+folly::Optional<std::string> spillerSpillPath() const
+int32_t shutdownOnsetSec() const
static constexpr std::string_view kBroadcasterDirectoryCreateConfig
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider refactoring the logic that reads the directory create config and builds DirectoryOptions into a shared helper (it’s almost identical in the spiller code) to avoid duplication.
- It might be worth adding error handling or logging around the filesystem mkdir call so that failures due to an invalid directoryCreateConfig are surfaced with context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider refactoring the logic that reads the directory create config and builds DirectoryOptions into a shared helper (it’s almost identical in the spiller code) to avoid duplication.
- It might be worth adding error handling or logging around the filesystem mkdir call so that failures due to an invalid directoryCreateConfig are surfaced with context.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Closing. We have a better solution for our internal use case, and it's unclear if anyone else would benefit from this. |
Summary:
Adds support for configuring the directory used for Storage-based Broadcasts in Velox.
The "config" is a transparent string, which is expected to be interpreted by the Filesystem.
Differential Revision: D87552571