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.

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

upgrade lifecycle handler #168

Merged
merged 4 commits into from Jul 16, 2022
Merged

upgrade lifecycle handler #168

merged 4 commits into from Jul 16, 2022

Conversation

il3ven
Copy link
Contributor

@il3ven il3ven commented Jul 11, 2022

This is a proposal for an upgrade in lifecycle handler. It solves two main problems.

Additional benefit:

  • We can now define a path for the crawler to take in advance.

@TimDaub Have a look at the implementation. I tested it with minimal data and it works. I couldn't test it completely because of no IPFS gateway access right now. I'll try to test more and iron out the bugs.

@TimDaub
Copy link
Collaborator

TimDaub commented Jul 11, 2022

@reimertz can we set up @il3ven with an IPFS gateway access?

) {
log(`Ending extractor strategy with name "${strategy.module.name}"`);
}
const graph = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is amazing, I wanted to have something like this since a while!

@TimDaub
Copy link
Collaborator

TimDaub commented Jul 11, 2022

general feedback is that please continue with your work this is really helpful. However, I haven't had time to look into detail of this change set as I've been trying to fix the crawler

@TimDaub
Copy link
Collaborator

TimDaub commented Jul 11, 2022

this may also resolve: #170

Copy link
Collaborator

@TimDaub TimDaub left a comment

Choose a reason for hiding this comment

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

hard requirement is that all new functionality must be unit tested

@TimDaub
Copy link
Collaborator

TimDaub commented Jul 11, 2022

hard requirement is that all new functionality must be unit tested

Added issue for this too: #174

@TimDaub
Copy link
Collaborator

TimDaub commented Jul 11, 2022

btw I have sequentialized the strategy calls but without defining a graph for now to hot fix the current crawl. But I think you should still go forward with this PR as it is the right thing to do. But here is my intermediate change that sadly causes conflicts on your branch now: 0de7b87

@TimDaub
Copy link
Collaborator

TimDaub commented Jul 11, 2022

@il3ven
Copy link
Contributor Author

il3ven commented Jul 13, 2022

I ran a small crawl using this PR. It ran successfully. I crawled 543 NFTs. The strategy was to limit the NFTs in web3subgraph-transformation. The crawl took 3m40s with a concurrency of 50. For IPFS I used ipfs.io/ipfs as my gateway.

Here is the crawl data if you are interested. data.zip

I think only unit tests are remaining to be added.

The new crawl path schema allows us to only run extractor or
transformer of a strategy.
also remove unit tests for functions not used anymore
@il3ven il3ven changed the base branch from main to dev July 16, 2022 10:02
@il3ven il3ven marked this pull request as ready for review July 16, 2022 10:02
@il3ven
Copy link
Contributor Author

il3ven commented Jul 16, 2022

Due to lack of reviewer, merging this branch to dev for now. So I can run the crawler without affecting master.

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.

None yet

2 participants