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

Remove ServerBuilder in favour of simple constructor on Server #25903

Open
hiltontj opened this issue Jan 23, 2025 · 0 comments
Open

Remove ServerBuilder in favour of simple constructor on Server #25903

hiltontj opened this issue Jan 23, 2025 · 0 comments
Labels

Comments

@hiltontj
Copy link
Contributor

Problem statement

The ServerBuilder type causes friction when extending the state that is provided to to the Server type.

A builder pattern seems unnecessary here. The number of generics tends to cause "type wars" when introducing changes. Since we only create one Server in a given instance, a simple constructor should suffice.

Proposed solution

Remove ServerBuilder in favour of a Server::new method that accepts a CreateServerArgs struct with whatever state is needed to create the server. This would likely adopt a lot of the logic from the existing SerberBuilder::build method:

pub async fn build(self) -> Server<T> {
let persister = Arc::clone(&self.persister.0);
let authorizer = Arc::clone(&self.authorizer);
let processing_engine = Arc::new(ProcessingEngineManagerImpl::new(
self.common_state.plugin_dir.clone(),
self.write_buffer.0.catalog(),
Arc::clone(&self.write_buffer.0),
Arc::clone(&self.query_executor.0),
Arc::clone(&self.time_provider.0) as _,
self.write_buffer.0.wal(),
));
processing_engine
.start_triggers()
.await
.expect("failed to start processing engine triggers");
self.write_buffer
.0
.wal()
.add_file_notifier(Arc::clone(&processing_engine) as _);
let http = Arc::new(HttpApi::new(
self.common_state.clone(),
Arc::clone(&self.time_provider.0),
Arc::clone(&self.write_buffer.0),
Arc::clone(&self.query_executor.0),
processing_engine,
self.max_request_size,
Arc::clone(&authorizer),
));
Server {
common_state: self.common_state,
http,
persister,
authorizer,
listener: self.listener.0,
}
}

Additional context

Consideration should be given to how this will integrate to enterprise, in cases where we need to add enterprise-specific state to the server. Currently, the builder is causing pain points there when merging from core to enterprise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant