This repository has been archived by the owner on Oct 25, 2024. It is now read-only.
fix: --embedded-database does not work when using fuel-indexer binary directly #892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lostman
force-pushed
the
maciej/813-fuel-indexer-embedded-database
branch
from
fe336ad to
f10df37
Compare
lostman
force-pushed
the
maciej/813-fuel-indexer-embedded-database
branch
from
d10b40e to
9c92a65
Compare
ra0x3
suggested changes
May 13, 2023
There was a problem hiding this comment.
- Looks mostly OK
- Left some minor comments
- Left a bigger comment about the feature flagging / service start change is gonna require manual testing steps
- Hopefully https://github.com/FuelLabs/fuel-indexer/issues will alleviate this a bit
| @@ -57,56 +100,82 @@ pub async fn exec(args: IndexerArgs) -> anyhow::Result<()> { | |||
|
|
|||
| let service_handle = tokio::spawn(service.run()); | |||
|
|
|||
| // for graceful shutdown | |||
There was a problem hiding this comment.
- Just a heads up here.
- I see the logic of how the service starts and is feature flagged has been changed
- Totally fine with that overall, but just an FYI that the manual testing to verify this works is gonna be a bit vigorous
- You will need to test:
- The fuel-indexer service starts with all/any features
--all-features,--no-default-features - The fuel-indexer service starts with
--local-fuel-node - The fuel-indexer-api-server starts with all/any features
--all-features,--no-default-features - Native execution starts with all/any features
--all-features,--no-default-features - Native execution starts with
--local-fuel-node- Native execution is here btw
- It's pretty much a copy/paste of how the non-native fuel-indexer service is started
- Docker builds still work
- Hello-indexer example
- Block-explorer example
- Hello-indexer-native example
- The fuel-indexer service starts with all/any features
- And we'd need to update the PR testing steps to reflect this
- You can see something somewhat similar over in fix: correct usage of
fuel-indexerfeature flags #772
- You can see something somewhat similar over in fix: correct usage of
|
After the fix yesterday, I re-ran various variations. Pasting outputs here:
|
lostman
force-pushed
the
maciej/813-fuel-indexer-embedded-database
branch
from
2622986 to
f3abd25
Compare
ra0x3
reviewed
May 24, 2023
There was a problem hiding this comment.
Spent some time reviewing this:
The following things work:
- Ctrl+C stops
cargo run --embedded-database forc index killdoes not stop forc-postgres (but still stops forc-index)- Ctrl+C stops
./target/release/fuel-indexer ./target/release/fuel-indexer --embedded-databaseworks./target/release/fuel-indexer --no-default-featuresstarts & stops- hello-native works
I was not able to get the following things to work:
- explorer-indexer works via docker
- hello-world works via docker
When trying to run the dockerized examples I'm consistently getting the following error
> docker compose up
block-explorer-fuel-indexer-1 | ./fuel-indexer: error while loading shared libraries: libssl.so.1.1: cannot open shared object file: No such file or directory
block-explorer-fuel-indexer-1 exited with code 127
My local image was built with the following command:
docker build -t fuel-indexer/local:latest -f deployment/Dockerfile .@lostman I'm assuming the Dockerized examples worked for you?
|
@ra0x3 We could switch to I'll re-check the Docker builds. |
@lostman Yep, we can definitely handle the postgres killing separately :) Lmk how the Docker thing works for you. @deekerno Could you also test the docker parts of this when you get a minute? I've already verified the other parts are working 👌🏽 |
|
With |
What is the error you're encountering? |
deekerno
previously approved these changes
Jun 5, 2023
…exer-embedded-database
After speaking about it with @lostman, I totally forgot that the fix itself would not be a part of the Docker compose workflow because it hasn't been included as part of an image yet. 😅 so I'm fine with this. Re-reviewing now. |
deekerno
approved these changes
Jun 6, 2023
ra0x3
approved these changes
Jun 6, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR makes
fuel-indexer startrun an embeddedpostgresdatabase when--embedded-databaseflag is specified.When
fuel-indexerexits, the embedded database is shut down. For this purpose,fuel-indexernow listens toSIGHUP/INT/TERMsignals.Testing steps
fuel-indexer--embedded-database--embedded-databaseand--local-fuel-nodeNote:
fuel-core-libmust be enabled.3
--embedded-databaseand--all-features,--no-default-featuresfuel-api-server--all-features,--no-default-featuresAnd check it works by visiting:
http://localhost:29987/api/playground/fuel_examples/explorer_indexer
Changelog
--embedded-databaseis specifiedforc index startpasses--embedded-databaseflag tofuel-indexerinstead of callingforc_postgres::commands::create::execopensslit's a fix for
Cannot build due to libssl.so.1.1 link error launchbadge/sqlx#473
"the issue is because procedural macro executed in a slightly different environment"
I think we're running into the same situation with our Docker build—we build with
cargo chefimage, but run onubuntu:22.04