Skip to content

Conversation

@nadin-Starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@github-actions
Copy link

github-actions bot commented Nov 25, 2025

@nadin-Starkware nadin-Starkware force-pushed the 11-25-apollo_storage_add_appstate_struct_for_axum_server branch 2 times, most recently from 49bdfa4 to b57a52a Compare November 25, 2025 13:34
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


crates/apollo_storage/src/storage_reader_server.rs line 62 at r2 (raw file):

    storage_reader: Arc<StorageReader>,
    request_handler: Arc<RequestHandler>,
    semaphore: Arc<Semaphore>,

That's not needed in this context; please remove.

Code quote:

semaphore: Arc<Semaphore>,

@nadin-Starkware nadin-Starkware changed the base branch from 11-25-apollo_storage_add_new_fn_for_serverconfig to main-v0.14.1 November 25, 2025 16:11
@nadin-Starkware nadin-Starkware force-pushed the 11-25-apollo_storage_add_appstate_struct_for_axum_server branch from b57a52a to 961790e Compare November 26, 2025 08:58
Copy link
Collaborator Author

@nadin-Starkware nadin-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)


crates/apollo_storage/src/storage_reader_server.rs line 62 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

That's not needed in this context; please remove.

Done.

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions


crates/apollo_storage/src/storage_reader_server.rs line 57 at r3 (raw file):

    RequestHandler: StorageReaderServerHandler<Request, Response>,
{
    storage_reader: Arc<StorageReader>,

This is cloneable, so no need to wrap it in an arc
(I think internally it uses arc anyway)

Code quote:

StorageReader

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @nadin-Starkware)


crates/apollo_storage/src/storage_reader_server.rs line 58 at r3 (raw file):

{
    storage_reader: Arc<StorageReader>,
    request_handler: Arc<RequestHandler>,

Any reason why RequestHandler wouldn't be Clone ? If not, please add it as a trait bound, and remove arc

Code quote:

Arc<RequestHandler>

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @nadin-Starkware)


crates/apollo_storage/src/storage_reader_server.rs line 84 at r3 (raw file):

    pub fn new(
        storage_reader: Arc<StorageReader>,
        request_handler: Arc<RequestHandler>,

Regardless of the changes above, can these be without arc, and wrapped internally?

Code quote:

        storage_reader: Arc<StorageReader>,
        request_handler: Arc<RequestHandler>,

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @nadin-Starkware)


crates/apollo_storage/Cargo.toml line 38 at r3 (raw file):

tempfile = { workspace = true, optional = true }
thiserror.workspace = true
tokio = { workspace = true, features = ["sync"] }

What requires this feature?

Code quote:

features = ["sync"] 

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.

4 participants