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.

Sign up for GitHub

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

Jump to bottom

substrate-offchain: upgrade hyper to v1 #5919

Merged
merged 1 commit into from
Oct 25, 2024
Merged

substrate-offchain: upgrade hyper to v1 #5919

merged 1 commit into from
Oct 25, 2024
+211 −146

Conversation

ShoyuVanilla
Copy link
Contributor

Closes #4896

@bkchr bkchr added the T17-primitives Changes to primitives that are not covered by any other label. label Oct 5, 2024
@github-actions github-actions bot requested a review from bkchr October 6, 2024 02:46
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 6, 2024

Review required! Latest push from author must always be reviewed

@ShoyuVanilla
Copy link
Contributor Author

ShoyuVanilla commented Oct 6, 2024 •
edited
Loading

I've rebased on master and fixed fmt fails with rustfmt nightly but still have to fix some tests are failing in CI (worked fine on my apple silicon mac)

niklasad1
niklasad1 reviewed Oct 7, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
niklasad1
niklasad1 reviewed Oct 7, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
ShoyuVanilla
ShoyuVanilla commented Oct 7, 2024
View reviewed changes
substrate/client/offchain/src/api/http.rs
type Receiver = mpsc::Receiver<Result<hyper::body::Frame<hyper::body::Bytes>, hyper::Error>>;

type HyperClient = client::Client<HttpsConnector<client::connect::HttpConnector>, Body>;
type LazyClient = Lazy<HyperClient, Box<dyn FnOnce() -> HyperClient + Send>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once rustc has stabilized TAIT, this boxing can be avoided.
The other option is implementing similar thing as once_cell::Lazy with once_cell::OnceCell and std::cell::Cell

Copy link
Member

Choose a reason for hiding this comment

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

yeah, this is fine

ShoyuVanilla
ShoyuVanilla commented Oct 7, 2024
View reviewed changes
substrate/client/offchain/src/api/http.rs
Ok(Self(Arc::new(Lazy::new(Box::new(|| {
let connector = builder.https_or_http().enable_http1().enable_http2().build();
client::Client::builder(TokioExecutor::new()).build(connector)
})))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above boxing is needed to resolve this; This should be a Lazy with closure, otherwise we cannot return std::io::Result<Self> and Self.0 becomes Lazy<std::io::Result<HyperClient>>, which is quite inconvenient to use

niklasad1
niklasad1 reviewed Oct 7, 2024
View reviewed changes
substrate/client/offchain/src/lib.rs Outdated
@@ -160,7 +160,7 @@ impl<RA, Block: traits::Block, Storage> OffchainWorkers<RA, Block, Storage> {
"offchain-worker".into(),
num_cpus::get(),
)),
shared_http_client: api::SharedClient::new(),
shared_http_client: api::SharedClient::new().unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this requires unwrap anyway...

@bkchr Is it ok to move the shared_http_client to the OffchainWorkerOptions type such that we can bail early from the service builder if this fails?

Copy link
Contributor Author

@ShoyuVanilla ShoyuVanilla Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I thought that all the unwrap() calls were inside the test codes but this was not 🙃 Maybe I should change the return type to std::io::Result<Self>?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should handle the error here and just pass SharedHttpClient in the OffChainWorkerOptions instead to avoid breaking this api.

but better to wait what @bkchr thinks is the best way :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I'll wait for 😃

Copy link
Member

Choose a reason for hiding this comment

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

I think we should handle the error here and just pass SharedHttpClient in the OffChainWorkerOptions instead to avoid breaking this api.

We will break the API any way 🙈 As we need to do it, I think returning Result from new sounds better to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should handle the error here and just pass SharedHttpClient in the OffChainWorkerOptions instead to avoid breaking this api.

We will break the API any way 🙈 As we need to do it, I think returning Result from new sounds better to me

Do you mean this new method, OffchainWorkers::new?

Copy link
Member

Choose a reason for hiding this comment

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

yes, ignore my comment and change OffchainWorkers::new() to return Result<Self, Error> instead of Self

@niklasad1
Copy link
Member

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 7, 2024 •
edited
Loading

@niklasad1 https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7524560 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 24-df17e9e7-773d-4c48-9dd6-3e6377ae0f4f to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Oct 7, 2024

@niklasad1 Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7524560 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7524560/artifacts/download.

niklasad1
niklasad1 approved these changes Oct 7, 2024
View reviewed changes
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Looks good to me modulo the unwrap stuff

@ShoyuVanilla
Copy link
Contributor Author

Formatted Cargo.toml. I'll write prdoc soon

@ShoyuVanilla ShoyuVanilla force-pushed the offchain-hyperv1 branch 3 times, most recently from 2e5457f to 5e798a3 Compare October 11, 2024 03:55
@ShoyuVanilla
Copy link
Contributor Author

I have no idea why continuous-integration/gitlab-zombienet-polkadot-smoke-0005-precompile-pvf-smoke fails 🤔
https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7556991

niklasad1
niklasad1 approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice work, LGTM

@ShoyuVanilla ShoyuVanilla force-pushed the offchain-hyperv1 branch 3 times, most recently from 745834b to 4647f23 Compare October 12, 2024 18:35
@ShoyuVanilla
Copy link
Contributor Author

The CI failure seems to be caused from docker image push timeout. I wonder if I should take some action for it 🤔

bkchr
bkchr approved these changes Oct 14, 2024
View reviewed changes
prdoc/pr_5919.prdoc Outdated

crates:
- name: sc-offchain
bump: minor
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bump: minor
bump: major

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I've applied this change

@bkchr bkchr enabled auto-merge October 15, 2024 07:28
@ShoyuVanilla
Copy link
Contributor Author

ShoyuVanilla commented Oct 15, 2024 •
edited
Loading

I'm rebasing to re-trigger failed required-actions

auto-merge was automatically disabled October 15, 2024 08:11

Head branch was pushed to by a user without write access

@niklasad1
Copy link
Member

I'm rebasing to re-trigger failed required-actions

You don't have to do that, that merge queue will merge it to master before running the tests anyway. By rebasing now you removed this from the merge queue.... Some job is probably flaky but don't rebase again and let's try to add it to the merge queue again.

@ShoyuVanilla
Copy link
Contributor Author

I'm rebasing to re-trigger failed required-actions

You don't have to do that, that merge queue will merge it to master before running the tests anyway. By rebasing now you removed this from the merge queue.... Some job is probably flaky but don't rebase again and let's try to add it to the merge queue again.

Oh, I thought that "required" checks were necessary 😅 Sorry

@niklasad1
Copy link
Member

Oh, I thought that "required" checks were necessary 😅 Sorry

Yeah, they are but these failed checks are really unrelated to your PR and "might" have been fixed on master concurrently, it's fine to rebase just avoid to do it when it's added to the merge queue.

@bkchr
Copy link
Member

bkchr commented Oct 15, 2024

@ShoyuVanilla please merge master

@bkchr bkchr enabled auto-merge October 15, 2024 11:11
@jasl
Copy link
Contributor

jasl commented Oct 21, 2024

Hi, this one seems still CI broken

@ShoyuVanilla
Copy link
Contributor Author

Hi, this one seems still CI broken

@jasl Is there anything that I should/could do for CI failure? I'm not familiar with CI and not sure the failure is related to this PR or not 😅

@niklasad1
Copy link
Member

niklasad1 commented Oct 25, 2024 •
edited
Loading

@jasl Is there anything that I should/could do for CI failure? I'm not familiar with CI and not sure the failure is related to this PR or not 😅

try to rebase one more time, sorry about this

@ShoyuVanilla
Copy link
Contributor Author

try to rebase one more time, sorry about this

No problem 😄

auto-merge was automatically disabled October 25, 2024 07:25

Head branch was pushed to by a user without write access

@niklasad1 niklasad1 added this pull request to the merge queue Oct 25, 2024
Merged via the queue into paritytech:master with commit 5a14285 Oct 25, 2024
193 of 195 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkchr bkchr bkchr approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

Assignees
No one assigned
Labels
T17-primitives Changes to primitives that are not covered by any other label.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

offchain http: upgrade hyper to v1
4 participants
@ShoyuVanilla @niklasad1 @bkchr @jasl