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

Orchestrator refactor #512

Merged
merged 19 commits into from
Jul 10, 2022
Merged

Orchestrator refactor #512

merged 19 commits into from
Jul 10, 2022

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Jun 30, 2022

No description provided.

@rach-id rach-id added the C: QGB label Jun 30, 2022
@rach-id rach-id self-assigned this Jun 30, 2022
@rach-id rach-id marked this pull request as draft June 30, 2022 15:01
@rach-id
moves enquiying logic to orchestrator
4460242
@rach-id
remove unnecessary new lines
a88dc8d
@rach-id
adds a signaling channel to notify senders + better context cancelling
4788074
@rach-id
remove unnecessary added config fields
09158cd
@rach-id
add new line
f87c7b3
@rach-id
docs
a8598c9
@rach-id
docs
d9f7b0a
@rach-id
logs
192740f
@rach-id
logs
5c3461c
@rach-id
query valset confirm instead of dc confirm in qgb e2e wait for orch t…
eadea40 
…o start
@rach-id
QGB E2e cosmetics (celestiaorg#515)
4406b88 
* cosmetics

* better waiting for e2e orchestrator

* use require instead of assert in some qgb e2e places

* restore the confirm check

* Update e2e/qgb/orchestrator_test.go

* Update e2e/qgb/orchestrator_test.go

* Update e2e/qgb/orchestrator_test.go

* Update e2e/qgb/orchestrator_test.go

* Update e2e/qgb/orchestrator_test.go

* Update e2e/qgb/orchestrator_test.go

* Update e2e/qgb/orchestrator_test.go

* Update e2e/qgb/orchestrator_test.go

* Update e2e/qgb/orchestrator_test.go

* Update e2e/qgb/orchestrator_test.go
evan-forbes
evan-forbes reviewed Jul 1, 2022
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

some minor nits, but overall a big 👍

probably about as clean as we're going to get! anything smaller and I think we're treading into over abstraction

x/qgb/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
x/qgb/orchestrator/orchestrator_command.go Outdated Show resolved Hide resolved
x/qgb/orchestrator/orchestrator_command.go Outdated Show resolved Hide resolved
x/qgb/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
x/qgb/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
x/qgb/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
rach-id
rach-id commented Jul 2, 2022
View reviewed changes
Copy link
Member Author

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

Questions :D

x/qgb/orchestrator/orchestrator.go
Comment on lines +356 to +362
// FIXME sdktypestx.BroadcastMode_BROADCAST_MODE_BLOCK waits for a block to be minted containing
// the transaction to continue. This makes the orchestrator slow to catchup.
// It would be better to just send the transaction. Then, another job would keep an eye
// if the transaction was included. If not, retry it. But this would mean we should increment ourselves
// the sequence number after each broadcasted transaction.
// We can also use BroadcastMode_BROADCAST_MODE_SYNC but it will also fail due to a non incremented
// sequence number.
Copy link
Member Author

Choose a reason for hiding this comment

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

@evan-forbes please when you have time, let me know what you think about this,

Copy link
Member

Choose a reason for hiding this comment

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

we might want to create larger txs with many sdk.Messages and still use broadcast mode sync just for simplicity, but I can definitely see where a more async approach would be desirable

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we create an issue for that?

x/qgb/orchestrator/orchestrator.go Show resolved Hide resolved
x/qgb/orchestrator/orchestrator.go Show resolved Hide resolved
x/qgb/orchestrator/orchestrator.go Outdated Show resolved Hide resolved
@rach-id
Copy link
Member Author

rach-id commented Jul 4, 2022

@evan-forbes ready for final review, after you're back. Also, please check the comments up.

@codecov-commenter
Copy link

Codecov Report

Merging #512 (7169d9a) into qgb-integration (458b6b2) will increase coverage by 0.97%.
The diff coverage is 44.85%.

@@                 Coverage Diff                 @@
##           qgb-integration     #512      +/-   ##
===================================================
+ Coverage            11.72%   12.70%   +0.97%     
===================================================
  Files                   65       56       -9     
  Lines                10881     9548    -1333     
===================================================
- Hits                  1276     1213      -63     
+ Misses                9515     8252    -1263     
+ Partials                90       83       -7
Impacted Files Coverage Δ
e2e/qgb/qgb_network.go 0.00% <0.00%> (ø)
x/qgb/orchestrator/test/mock_querier.go 16.27% <16.27%> (ø)
x/qgb/abci.go 43.39% <16.66%> (-3.67%) ⬇️
x/qgb/orchestrator/test/mock_retrier.go 40.00% <40.00%> (ø)
x/qgb/orchestrator/test/test_utils.go 60.52% <60.52%> (ø)
x/qgb/orchestrator/test/mock_broadcaster.go 100.00% <100.00%> (ø)
x/qgb/orchestrator/orchestrator.go
x/qgb/orchestrator/orchestrator_command.go
x/qgb/orchestrator/querier.go
x/qgb/orchestrator/config.go
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 458b6b2...7169d9a. Read the comment docs.

This was referenced Jul 4, 2022
Merged
Closed
@rach-id rach-id marked this pull request as ready for review July 5, 2022 14:08
This was referenced Jul 10, 2022
Closed
Closed
@rach-id
Copy link
Member Author

rach-id commented Jul 10, 2022

merging :D

@rach-id rach-id merged commit 41d0ae7 into celestiaorg:qgb-integration Jul 10, 2022
@rach-id rach-id deleted the orchestrator_refactor branch July 10, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

Assignees

@rach-id rach-id

Labels
None yet
Projects
No open projects
Celestia Node
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rach-id @codecov-commenter @evan-forbes