Persistent subgraphs #513

timmclean · 2018-10-23T15:43:37Z

I believe with this PR we can leave ethsf behind?

Resolves #404

Major changes:

Schemas, SubgraphInstances, BlockStreams, etc no longer have a subgraph name associated with them. Just a SubgraphId.
Store has 7 new methods related to reading/writing/deleting subgraph name->ID mappings, access tokens, etc. This information was previously stored in-memory in graph-node.
Components that used to track subgraph name->ID mappings in-memory now have an Arc<Store> and have been rewritten to use that (e.g. JSON RPC service, SubgraphProvider deploy/remove)
SubgraphProvider::new is now SubgraphProvider::init. It retrieves a list of subgraph names from the Store and starts those subgraphs.
MockStore has been extended with mock implementations for some of the new methods. MockStore is used in a bunch of new places now that more components use a Store.

SubgraphProvider deploy and remove are hopefully pretty readable. deploy is essentially:

Resolve the subgraph manifest
Call SubgraphProvider::remove to undeploy the subgraph name if it already exists
Add subgraph ID directives
Write this subgraph name->ID mapping to the Store
Look for other names in the Store pointing at this ID. If none, start processing on this subgraph, otherwise, done (subgraph is already be running).

remove is essentially:

Read the subgraph name->ID mapping from the Store
Delete the name->ID mapping in the Store
Look for other names in the Store pointing at this ID. If none, shut down processing on this subgraph, otherwise, done (leave the subgraph running).

timmclean · 2018-10-23T17:53:20Z

Opening this up for code review now since there's a lot here, despite the fact that it's not done. Most of the work from the last few days has been trying to remove the assumption that each subgraph has one and only one name... would be interested in some feedback on whether that's the right approach. @Jannis

Update: done now!

leoyvens · 2018-10-29T19:19:37Z

In an architecture with a dedicated name service the node would not be managing names at all, but we still want built-in name management for standalone nodes. To make this mode switching easier, what do you think of extracting the name logic in SubgraphProvider to a SubgraphNames component (or whatever name you prefer). SubgraphNames would have methods analogous to deploy and remove, that would return whether or not the subgraph should be deployed or removed. When running with a dummy SubgraphNames, the name is completely ignored and the subgraph is always deployed or removed. @timmclean makes sense? What do you think?

timmclean · 2018-10-29T19:32:51Z

Ah interesting. That could work. What would this look like externally? Right now I believe the JSON RPC service has deploy/remove/authorize/list. Should we use the existing verbs, revise them to make them more flexible, or add a separate JSON RPC service with a new set?

leoyvens · 2018-10-29T19:43:47Z

@timmclean I wouldn't touch that layer at all yet, we're gonna need to figure that out later (hopefully it will be graphql mutations rather than json-rpc) but for now I just wanna make sure it will be clean and easy to turn off the code that writes and removes names.

timmclean · 2018-10-29T20:19:37Z

So how would deploy and remove work in SubgraphProvider in that case? They both have name arguments... would we pass an empty string? None?

timmclean · 2018-10-29T21:15:27Z

Planned refactor for separating name functionality from SubgraphProvider:

In SubgraphProvider:
- Does not depend on Store.
- Track a HashSet of SubgraphIds that are currently running in this graph node instance.
- deploy(name, link) will become start(link). Error if subgraph ID already in the HashSet. Add ID to HashSet.
- remove(name) will become stop(id). Error if subgraph ID not in the HashSet. Remove ID from HashSet.
- Emits events just like current SubgraphProvider.
New component: SubgraphProviderWithNames
- Does depend on Store.
- Used by JSON RPC service.
- deploy(name, link): same functionality as current SubgraphProvider::deploy(name, link)
- remove(name): same functionality as current SubgraphProvider::remove(name)
- deploy and remove call start and stop on the SubgraphProvider as needed.

@leodasvacas

leoyvens · 2018-10-29T21:30:41Z

@timmclean thanks for writing out the plan, looks solid!

timmclean · 2018-10-30T00:33:07Z

Refactor done!

leoyvens

Overall this is great! Left some comments.

core/src/subgraph/provider_with_names.rs

graph/src/components/subgraph/provider.rs

core/src/subgraph/provider.rs

core/src/subgraph/provider_with_names.rs

graph/src/components/store.rs

server/http/src/service.rs

graph/src/components/store.rs

timmclean · 2018-10-30T16:50:21Z

@leodasvacas I pushed some commits, how does this look?

timmclean · 2018-10-31T00:41:13Z

Latest commit adds separate endpoints to server/http and server/websocket for accessing subgraphs by ID or name. E.g. http://localhost:8000/by-name/adchain or http://localhost:8000/by-id/QmaMUYb8GBZBrK6664P822xYyfN2nqR5VVfpkWFyvtwyD9

leoyvens

I like the new url scheme!

leoyvens · 2018-10-31T12:16:18Z

server/http/src/service.rs

+                        service.handle_not_found()
+                    },
+                ),
+        )


We now print urls on deploy but I guess it's still nice to have this back. Could you also bring back the comment that explains what this does?

Oh true, yeah, I don't feel too strongly on having a redirect or not! I was tempted to put in a quicky plaintext list of subgraph names...

Comment added though

server/websocket/src/server.rs

Jannis

Great! A few small comments, some just notes, a couple changes I'd like to see.

Jannis · 2018-10-31T20:56:42Z

core/src/subgraph/provider.rs

+            .event_sink
+            .clone()
+            .send(SubgraphProviderEvent::SubgraphStop(id))
+            .map_err(|e| panic!("failed to forward subgraph removal: {}", e))


Is removal still the best word here?

Jannis · 2018-10-31T21:29:36Z

node/src/main.rs

+            logger.clone(),
+            Arc::new(subgraph_provider),
+            store.clone(),
+        ).wait()


This wait... can it not cause freezing (e.g. if something inside init were to spawn something)?

Based on my current level of tokio understanding, I think it's fine... async_main is already running inside the context of a tokio runtime, and tokio can use other threads

@leodasvacas ?

Jannis · 2018-10-31T21:45:02Z

server/http/src/service.rs

+                .header(
+                    header::LOCATION,
+                    header::HeaderValue::from_str(destination)
+                        .expect("invalid redirect destination"),


Is this something that should make the node panic?

Jannis · 2018-10-31T21:47:45Z

server/json-rpc/src/lib.rs

+
+        return Err(json_rpc_error(
+            JSON_RPC_UNAUTHORIZED_ERROR,
+            "API key is invalid".to_owned(),


This should probably be Invalid access token, since API key is not what we call this any more.

makes sense!

Jannis · 2018-10-31T21:50:37Z

server/json-rpc/src/lib.rs

@@ -160,36 +196,62 @@ impl<T: SubgraphProvider> JsonRpcServer<T> {
    ) -> Result<Value, jsonrpc_core::Error> {
        info!(self.logger, "Received subgraph_authorize request"; "params" => params.to_string());
        Self::require_master_token(auth)?;
-        *self.subgraph_api_keys.write().unwrap() = params.subgraph_api_keys;
+
+        for (subgraph_name, access_token) in params.subgraph_api_keys {


Let's rename subgraph_api_keys to subgraph_access_tokens as well.

This is a breaking change in the JSON-RPC API but that's fine IMHO.

server/websocket/src/server.rs

Jannis · 2018-10-31T21:54:57Z

store/postgres/src/store.rs

@@ -434,6 +434,104 @@ impl Store {
 }

 impl StoreTrait for Store {
+    fn authorize_subgraph_name(&self, name: String, new_access_token: String) -> Result<(), Error> {


We're putting a bit much into the Store. At some point it will be worth thinking about splitting this up into separate traits and separate implementations. But not right now.

Agreed. We should have a separate store component for subgraph name info

Jannis · 2018-10-31T21:57:14Z

store/postgres/src/store.rs

+            None => {
+                debug!(
+                    self.logger,
+                    "Subgraph name {:?} has no associated access token. Access denied by default.",


This should use structured logging:

debug!( self.logger, "Subgraph has no associated access token. Access denied by default."; "subgraph_name" => &subgraph_name );

Also: you're passing in access_token instead of subgraph_name anyway. 😉

… subgraph name and ID

More work to remove the assumption that each subgraph ID is associated with a single name.

…ption of 1 name per subgraph

…re traits

based on code review feedback

…usion in URLs Adds pages at /by-id/<subgraph ID> and /by-name/<subgraph name>

timmclean · 2018-10-31T22:53:27Z

I think I got everything!

timmclean added enhancement New feature or request area/json-rpc area/subgraphs area/store labels Oct 23, 2018

timmclean self-assigned this Oct 23, 2018

ghost added pending review review labels Oct 23, 2018

timmclean requested review from leoyvens, fordN and Jannis October 23, 2018 17:50

timmclean force-pushed the tim/persistent-subgraphs branch 2 times, most recently from ce01583 to 4141d40 Compare October 27, 2018 00:32

timmclean changed the title ~~WIP persistent subgraphs~~ Persistent subgraphs Oct 27, 2018

timmclean added this to the Hosted Service milestone Oct 29, 2018

leoyvens requested changes Oct 30, 2018

View reviewed changes

timmclean force-pushed the tim/persistent-subgraphs branch from b8b918d to a99c3c3 Compare October 30, 2018 20:35

leoyvens requested changes Oct 31, 2018

View reviewed changes

leoyvens previously approved these changes Oct 31, 2018

View reviewed changes

Jannis requested changes Oct 31, 2018

View reviewed changes

core, graph, node, server, store: Persist subgraphs

3925ad9

timmclean added 11 commits October 31, 2018 18:25

WIP: schema name refactor

870f27d

core, graph, node, runtime, server: Remove 1-1 correspondence between…

dbdccb9

… subgraph name and ID

core, graphql, server: Fix some of SubgraphProvider tests

21bcdd1

core, graph, mock, server: Continue persist subgraphs refactor

1703a1f

More work to remove the assumption that each subgraph ID is associated with a single name.

core, datasource, graph, mock, runtime, server: Finish removing assum…

54c00a9

…ption of 1 name per subgraph

core, graph, node, server: Hide name resolution from SubgraphProvider

ba3005e

core, datasource, graph, runtime, server: Move 'static markers to Sto…

a58b2ad

…re traits

core, graph: Clean up SubgraphProvider

f109c0f

based on code review feedback

node, server/{http,json-rpc,websocket}: Prevent subgraph name/ID conf…

f2be844

…usion in URLs Adds pages at /by-id/<subgraph ID> and /by-name/<subgraph name>

server/http: Add comment to index page redirect

446af27

core, server, store: Integrate feedback from code review

15f6f2b

timmclean dismissed leoyvens’s stale review via 15f6f2b October 31, 2018 22:51

timmclean force-pushed the tim/persistent-subgraphs branch from 6f6e07a to 15f6f2b Compare October 31, 2018 22:51

Jannis approved these changes Oct 31, 2018

View reviewed changes

timmclean merged commit cf98685 into master Oct 31, 2018

ghost removed pending review review labels Oct 31, 2018

timmclean deleted the tim/persistent-subgraphs branch October 31, 2018 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persistent subgraphs #513

Persistent subgraphs #513

timmclean commented Oct 23, 2018 •

edited

Loading

timmclean commented Oct 23, 2018 •

edited

Loading

leoyvens commented Oct 29, 2018

timmclean commented Oct 29, 2018

leoyvens commented Oct 29, 2018

timmclean commented Oct 29, 2018

timmclean commented Oct 29, 2018 •

edited

Loading

leoyvens commented Oct 29, 2018 •

edited

Loading

timmclean commented Oct 30, 2018

leoyvens left a comment

timmclean commented Oct 30, 2018 •

edited

Loading

timmclean commented Oct 31, 2018

leoyvens left a comment

leoyvens Oct 31, 2018

timmclean Oct 31, 2018

Jannis left a comment

Jannis Oct 31, 2018

Jannis Oct 31, 2018

timmclean Oct 31, 2018

Jannis Oct 31, 2018

Jannis Oct 31, 2018

timmclean Oct 31, 2018

Jannis Oct 31, 2018

Jannis Oct 31, 2018

Jannis Oct 31, 2018

timmclean Oct 31, 2018

Jannis Oct 31, 2018

timmclean commented Oct 31, 2018

Persistent subgraphs #513

Persistent subgraphs #513

Conversation

timmclean commented Oct 23, 2018 • edited Loading

timmclean commented Oct 23, 2018 • edited Loading

leoyvens commented Oct 29, 2018

timmclean commented Oct 29, 2018

leoyvens commented Oct 29, 2018

timmclean commented Oct 29, 2018

timmclean commented Oct 29, 2018 • edited Loading

leoyvens commented Oct 29, 2018 • edited Loading

timmclean commented Oct 30, 2018

leoyvens left a comment

Choose a reason for hiding this comment

timmclean commented Oct 30, 2018 • edited Loading

timmclean commented Oct 31, 2018

leoyvens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jannis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmclean commented Oct 31, 2018

timmclean commented Oct 23, 2018 •

edited

Loading

timmclean commented Oct 23, 2018 •

edited

Loading

timmclean commented Oct 29, 2018 •

edited

Loading

leoyvens commented Oct 29, 2018 •

edited

Loading

timmclean commented Oct 30, 2018 •

edited

Loading