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

[crates] Rename crates B through D inclusive #5788

Merged
merged 1 commit into from
Dec 7, 2022
Merged

[crates] Rename crates B through D inclusive #5788

merged 1 commit into from
Dec 7, 2022

Conversation

banool
Copy link
Contributor

@banool banool commented Dec 6, 2022 •
edited
Loading

Description

Doing more this time, otherwise I'll have to make 50+ PRs. This doesn't include channel because it's not just a simple rename due to clashes with the name aptos_channel.

Test Plan

I'm running scripts/rust_lint.sh locally. Now CI.

@banool banool changed the title [crates] Rename backup-service to aptos-backup-service [crates] Rename crates B through D Dec 6, 2022
@banool banool force-pushed the banool/crates_backup_service branch from 34e2270 to 0ecbe5f Compare December 6, 2022 19:46
@banool banool changed the title [crates] Rename crates B through D [crates] Rename crates B through D inclusive Dec 7, 2022
@banool banool marked this pull request as ready for review December 7, 2022 11:21
@banool banool force-pushed the banool/crates_backup_service branch from 0ecbe5f to d459698 Compare December 7, 2022 11:21
@banool banool force-pushed the banool/crates_backup_service branch from d459698 to 263b4e2 Compare December 7, 2022 11:24
@banool
Copy link
Contributor Author

banool commented Dec 7, 2022

This fails for me, but I feel like it shouldn't be:

python3 scripts/check-cryptohasher-symbols.py

It says for example that the name BlockData is used twice, by aptos_consensus_types and consensus_types, but the latter doesn't exist anymore as of this PR.

Notably this doesn't fail locally, it only fails in CI.

I fixed up a doc string consensus/src/block_storage/block_tree.rs as part of debugging this.

@banool banool force-pushed the banool/crates_backup_service branch from 263b4e2 to 85a38b3 Compare December 7, 2022 11:43
JoshLind
JoshLind approved these changes Dec 7, 2022
View reviewed changes
Copy link
Contributor

@JoshLind JoshLind left a comment

Choose a reason for hiding this comment

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

Thanks @banool! Looks reasonable to me. Let's see if owners/others have concerns.

aptos-move/framework/aptos-framework/doc/voting.md
@@ -107,8 +107,9 @@ Extra metadata (e.g. description, code url) can be part of the ProposalType stru
Currently, we have three attributes that are used by the voting flow.
1. RESOLVABLE_TIME_METADATA_KEY: this is uesed to record the resolvable time to ensure that resolution has to be done non-atomically.
2. IS_MULTI_STEP_PROPOSAL_KEY: this is used to track if a proposal is single-step or multi-step.
3. IS_MULTI_STEP_PROPOSAL_IN_EXECUTION_KEY: this attribute only exists for and applies to multi-step proposals. The value is used to
indicate if a multi-step proposal is in execution. If yes, we will disable further voting for this multi-step proposal.
3. IS_MULTI_STEP_PROPOSAL_IN_EXECUTION_KEY: this attribute only applies to multi-step proposals. A single-step proposal will not have
Copy link
Contributor

@JoshLind JoshLind Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Where did these come from? I assume they aren't checked in? If so, someone should follow up one of these days 😄 I see this a lot too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh yes a timeless classic: https://aptos-org.slack.com/archives/C03N9HNSUB1/p1668643130238119. I haven't had time to look into it yet.

@JoshLind
Copy link
Contributor

JoshLind commented Dec 7, 2022 •
edited
Loading

It says for example that the name BlockData is used twice, by aptos_consensus_types and consensus_types, but the latter doesn't exist anymore as of this PR.

Maybe @alinush or @zekun000 has thoughts? 😄

@alinush
Copy link
Contributor

alinush commented Dec 7, 2022

It says for example that the name BlockData is used twice, by aptos_consensus_types and consensus_types, but the latter doesn't exist anymore as of this PR.

Maybe @alinush or @zekun000 has thoughts? 😄

@banool and @zjma: If there are two identically-named BlockData structs that are being signed, that would be very bad, and that's why @zjma's check-cryptohasher-symbols.py script is complaining.

If this problem arose because of a failure to delete some old code that was renamed, that's okay.

But I'm not sure I understand how check-cryptohasher-symbols.py can complain about BlockData in consensus_types if it doesn't exist... @banool can you clarify?

Also, is it complaining about other things too?

@banool
Copy link
Contributor Author

banool commented Dec 7, 2022

@alinush I think perhaps it was some transient problem, because upon rebasing it seems to be passing in CI now. The problem I saw was it failed in CI but passed locally.

@banool banool force-pushed the banool/crates_backup_service branch from 85a38b3 to 7d6e8e2 Compare December 7, 2022 22:27
@banool banool enabled auto-merge (squash) December 7, 2022 22:41
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

✅ Forge suite land_blocking success on 7d6e8e2caf8727964ae359bf19f61a3fe1ead460

performance benchmark with full nodes : 6531 TPS, 6072 ms latency, 14500 ms p99 latency,(!) expired 1120 out of 2789940 txns
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7d6e8e2caf8727964ae359bf19f61a3fe1ead460

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7d6e8e2caf8727964ae359bf19f61a3fe1ead460 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7456 TPS, 5206 ms latency, 7200 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 7d6e8e2caf8727964ae359bf19f61a3fe1ead460
compatibility::simple-validator-upgrade::single-validator-upgrade : 4516 TPS, 8954 ms latency, 11600 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 7d6e8e2caf8727964ae359bf19f61a3fe1ead460
compatibility::simple-validator-upgrade::half-validator-upgrade : 4691 TPS, 8829 ms latency, 12700 ms p99 latency,no expired txns
4. upgrading second batch to new version: 7d6e8e2caf8727964ae359bf19f61a3fe1ead460
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6780 TPS, 5730 ms latency, 10600 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7d6e8e2caf8727964ae359bf19f61a3fe1ead460 passed
Test Ok

@banool banool merged commit bb60c54 into main Dec 7, 2022
@banool banool deleted the banool/crates_backup_service branch December 7, 2022 23:30
areshand pushed a commit to areshand/aptos-core-1 that referenced this pull request Dec 18, 2022
@banool @areshand
[crates] Rename backup-service to aptos-backup-service (aptos-labs#5788)
de56335
@Markuze Markuze mentioned this pull request Dec 26, 2022
Closed
@Markuze Markuze mentioned this pull request Jan 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msmouse msmouse msmouse approved these changes

@JoshLind JoshLind JoshLind approved these changes

@lightmark lightmark Awaiting requested review from lightmark lightmark is a code owner

@bchocho bchocho Awaiting requested review from bchocho bchocho is a code owner

@gregnazario gregnazario Awaiting requested review from gregnazario gregnazario is a code owner

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@movekevin movekevin Awaiting requested review from movekevin movekevin is a code owner

@wrwg wrwg Awaiting requested review from wrwg wrwg is a code owner

@ibalajiarun ibalajiarun Awaiting requested review from ibalajiarun ibalajiarun is a code owner

@geekflyer geekflyer Awaiting requested review from geekflyer geekflyer is a code owner

@sasha8 sasha8 Awaiting requested review from sasha8 sasha8 is a code owner

@gelash gelash Awaiting requested review from gelash gelash is a code owner

@zekun000 zekun000 Awaiting requested review from zekun000 zekun000 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@banool @JoshLind @alinush @msmouse