cylc / cylc-ui

Web app for monitoring and controlling Cylc workflows
https://cylc.github.io
GNU General Public License v3.0
37 stars 27 forks source link

WorkflowService/GQuery will merge subscription+query as a subscription #417

Closed kinow closed 3 years ago

kinow commented 4 years ago

Describe the bug

Not sure if really a bug, or more like a user error (probably the latter @oliver-sanders ? I think I might be doing something wrong when calling it) but if you have the following scenario:

The final result is: subscription { workflows } (I think it keeps the left side, and updates with different values from the right side of the operation?)

The issue happened while I was testing ApolloClient with the new deltas in @dwsutherland 's branch. I changed from subscription to query in the Tree view, and was expecting to see an HTTP request.

But instead ended up with a new operation in the WebSocket channel. Debugging it I noticed that the final query had been merged as subscription.

Release version(s) and/or repository branch(es) affected?

Steps to reproduce the bug

Expected behavior

Not sure what is the expected behavior here. I would expect to be able to use queries and subscriptions. Maybe the service needs two different methods, one to request data, the other to subscribe?

Or maybe just one to request data, with an extra option to also subscribe to updates with a subscription (that's what ApolloClient does I guess)

Screenshots

Additional context

I was changing it to experiment with the pattern from Apollo Client documentation, where the client asks for the large data in an HTTP request, then subscribes for updates via WebSocket.

1*KwnMO21k0d3UbyKWnlbeJg

My understanding is that it's better to avoid initiating a subscription with 1 message for a query. i.e. if you run a subscription with query { workflows }, it works, but you get all the tooling for a subscription, to get just 1 query response. The HTTP query is cheaper/faster, as it skips the operations/queues/extra syncio coroutines/etc and just goes straight to the resolver/schema.

Pull requests welcome! This is an Open Source project - please consider contributing a bug fix yourself (please read CONTRIBUTING.md before starting any work though).

kinow commented 4 years ago

Not a blocker, as for now I've simply renamed everything to query {}. And am experimenting with the subscribeToMore to create a subscription for the query of the Tree view, handling deltas.

oliver-sanders commented 4 years ago

Kinda user error in that we merge things we want to subscribe to, if we want a one-off query then we will require a different interface.

hjoliver commented 4 years ago

(Sorry if this is a stupid question, but why does the tree cpt have query instead of a subscription here?)

kinow commented 4 years ago

Not stupid at all, I should have explained this issue occurred while working on incremental updates. For incremental updates, we use 1 query + 1 subscription. The query retrieves the larger initial chunk of data (all the workflow information), and the subscription brings only the bits that were updated (add/remove/edit).

That's how ApolloClient works. Its API has a function to call subscribeToMore after you initiate a query. The alternative would be to try to use a subscription for the initial data, then ignore any updates, and listen only to updates via the other subscription.

Query:

query {
  workflows (ids: ["kinow|five"]) {
    id
    cyclePoints {}
    familyProxies {}
    taskProxies{}
  }
}

Subscription (more or less like this I think, it's being updated in the PR's for flow&uiserver):

subscription {
  deltas (ids: ["kinow|five"]) {
    pruned {}
    updated {}
    added{}
  }
}
hjoliver commented 4 years ago

Ah right (I remember that now). That seems sensible to me. So how is this "user error" then?

kinow commented 4 years ago

Ah right (I remember that now). That seems sensible to me. So how is this "user error" then?

The user in the case being me :grimacing: there should be some other way around it. I am probably doing something stupid and instead the views/components should be organized/created in another way I guess.

hjoliver commented 4 years ago

But @oliver-sanders response above suggests he thinks we don't need a one-off query to start the subscription?:

if we want a one-off query then we will require a different interface.

kinow commented 4 years ago

But @oliver-sanders response above suggests he thinks we don't need a one-off query to start the subscription?:

if we want a one-off query then we will require a different interface.

Yup, I wrote the code in the incremental updates branch (no PR for that yet, but it's on my deltas-1 branch) following the documentation of ApolloClient (the image I used in this issue description is from their docs too IIRC).

But all my code could be based on wrong assumptions. Maybe I should have done things in a different way, to avoid having this issue in the first place. The work on that branch is not the final version, it's more to confirm what issues we may have when adding the deltas to the UI.

This is one of these issues. If we are to use the ApolloClient interface, of 1 Query + 1 Subscription to receive updates, then the current WorkflowService/GQuery API would need some changes I guess. Or we adopt a different approach. Open to suggestions on how to handle this to avoid this with deltas and avoid this issue.