Skip to content

Hydra subscription fix#4413

Merged
mnaamani merged 2 commits intoJoystream:carthagefrom
Lezek123:hydra-subscription-fix
Nov 7, 2022
Merged

Hydra subscription fix#4413
mnaamani merged 2 commits intoJoystream:carthagefrom
Lezek123:hydra-subscription-fix

Conversation

@Lezek123
Copy link
Copy Markdown
Contributor

Companion for: Joystream/hydra#511

@vercel
Copy link
Copy Markdown

vercel bot commented Oct 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
pioneer-testnet ⬜️ Ignored (Inspect) Nov 2, 2022 at 0:41AM (UTC)

Copy link
Copy Markdown
Contributor

@zeeshanakram3 zeeshanakram3 left a comment

Choose a reason for hiding this comment

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

I have tested the PR, works as expected. Just one comment the processor_events_log entity still exists and is being continuously populated. Shouldn't we just get rid of that?

@Lezek123
Copy link
Copy Markdown
Contributor Author

Lezek123 commented Nov 2, 2022

Just one comment the processor_events_log entity still exists and is being continuously populated. Shouldn't we just get rid of that?

It's still used by the processor to persist information about its state across runs. For example functions like loadState and countProcessedEvents rely on its existence.

@zeeshanakram3
Copy link
Copy Markdown
Contributor

It's still used by the processor to persist information about its state across runs. For example functions like loadState and countProcessedEvents rely on its existence.

Looks good then. I have merged the corresponding hydra PR and published the packages. Please update this PR now.

@Lezek123 Lezek123 marked this pull request as ready for review November 2, 2022 12:42
@Lezek123
Copy link
Copy Markdown
Contributor Author

Lezek123 commented Nov 3, 2022

The failing test should be ok after #4421

Copy link
Copy Markdown
Contributor

@zeeshanakram3 zeeshanakram3 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

The implementation looks correct, based on the new hydra feature. To make use of the new endpoint through our deployment scripts its a simple update to the caddy config files, will need a slightly modified setup on our production nodes with linode load balancer.

PROCESSOR_INDEXER_GATEWAY=http://hydra-indexer-gateway:4000/graphql

# State update endpoint used by prcessor (to send state updates)
STATE_UPDATE_ENDPOINT=http://graphql-server:8082/update-processor-state
Copy link
Copy Markdown
Member

@mnaamani mnaamani Nov 7, 2022

Choose a reason for hiding this comment

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

Doesn't this need to be passed in env for the processor? (As in the e2e test setup in hydra repo)
Or is it being passed through .env file through use of the env_file directive ?

env_file:
      # relative to working directory where docker-compose was run from
      - .env

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the default using 'localhost' probably doesn't work on mac with docker desktop..

    STATE_UPDATE_ENDPOINT: str({
      devDefault: 'http://localhost:8082/update-processor-state',
    }),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or is it being passed through .env file through use of the env_file directive ?

Yes, it should be passed because of this directive, it doesn't need to be explicitly named in environment

the default using 'localhost' probably doesn't work on mac with docker desktop..

I'm aware it doesn't work in some setups, but it's just the default. I followed the approach already used for indexer endpoint, ie.: INDEXER_ENDPOINT_URL: str({ devDefault: 'http://localhost:4001' }),

@mnaamani
Copy link
Copy Markdown
Member

mnaamani commented Nov 7, 2022

@Lezek123 How can I test the endpoint? When I do a simple GET request I get an error logged:

[ 'Unauthorized access on /update-processor-state: 172.18.0.1' ]

@Lezek123
Copy link
Copy Markdown
Contributor Author

Lezek123 commented Nov 7, 2022

@Lezek123 How can I test the endpoint? When I do a simple GET request I get an error logged:

This is an additional security measure, so that only the processor is allowed to send update requests to this endpoint.
Also only POST requests are supported by this endpoint, as it is just for pushing the updates, not for receiving any data.

If you want to test pushing from localhost (via Postman app for example) you can set PROCESSOR_HOST=localhost in .env (in this case the processor will not be able to push any updates, but you will be able to push from localhost).

@mnaamani
Copy link
Copy Markdown
Member

mnaamani commented Nov 7, 2022

@Lezek123 How can I test the endpoint? When I do a simple GET request I get an error logged:

This is an additional security measure, so that only the processor is allowed to send update requests to this endpoint. Also only POST requests are supported by this endpoint, as it is just for pushing the updates, not for receiving any data.

If you want to test pushing from localhost (via Postman app for example) you can set PROCESSOR_HOST=localhost in .env (in this case the processor will not be able to push any updates, but you will be able to push from localhost).

Thanks for clarification, I ran some tests and works as expected:

configured STATE_UPDATE_ENDPOINT=http://host.docker.internal:8090/

$ nc -l 8090
POST / HTTP/1.1
Accept: application/json, text/plain, */*
Content-Type: application/json
User-Agent: axios/1.1.3
Content-Length: 110
Accept-Encoding: gzip, deflate, br
Host: host.docker.internal:8090
Connection: close

{"state":{"indexerHead":8,"chainHead":14,"lastScannedBlock":1,"lastProcessedEvent":"0000000001-000002-c112c"}}

@mnaamani mnaamani merged commit 8e74805 into Joystream:carthage Nov 7, 2022
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.

3 participants