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

dbmss info subscription #337

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Conversation

jk05
Copy link

@jk05 jk05 commented Oct 4, 2021

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

  • Adds a GraphQL Subscription for the dbmss info command
  • Added graphql-subscriptions and chokidar in order to set up a watcher looking for neo4j.pid files being created / unlinked to establish dbms start / stop and publish an event

What is the current behavior?

  • No GraphQL subscriptions

What is the new behavior?

  • Can subscribe to watchInfoDbmss to get DbmsInfo for a relevant DBMS started / stopped

Does this PR introduce a breaking change?

No

Other information:

  • Cannot scroll through the result pane of an subscription graphql/graphql-playground#1021 - if testing on GraphQL playground, can use a CSS change to be able to scroll. Should be fixed in newer versions.
  • Left comments originating from NestJS docs around suggestions which we can implement in a later dev phase, shouldn't be a blocker right now
  • Main issue is handling unsubscribing - removing the watcher etc. There is some discussion How to clean up resources when subscriptions end? nestjs/graphql#186 and I tried to implement it but couldn't get it to work yet - would prefer to get some ideas on this before I/we look into this any further as Im not sure I love this solution. Should we leave this for now and come back to it in another dev phase? Or should we put time into it now?
  • Haven't looked into testing GraphQL subscriptions yet. Will pick it up as part of this work or in cool down.

@jk05 jk05 added the help wanted Extra attention is needed label Oct 4, 2021
@jk05 jk05 changed the title initial dbmss info subscription dbmss info subscription Oct 4, 2021
@Context('environment') environment: Environment,
): Promise<AsyncIterator<unknown, any, undefined>> {
// @todo: this isn't fleshed out yet and is simplistic
// @todo: how to detect or handle unsubsribe? how to handle multiple watchers for an env?
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth looking into this: nestjs/graphql#186 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I looked into this. I couldn't get it to work yet but can take another look.

Copy link
Author

Choose a reason for hiding this comment

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

Left a note in the PR description. Just wondering if I should look into this and get it working now? Or save for next phase?

});

const eventHandler = (e: string) => {
const dbmsIdCheck = e.match(/dbms-([^/]*)/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const dbmsIdCheck = e.match(/dbms-([^/]*)/);
const dbmsIdCheck = e.match(/dbms-([^/]+)/);

// @todo: how to detect or handle unsubsribe? how to handle multiple watchers for an env?
if (!environmentWatchers.get(environment.id)) {
// set watcher for environment
const setWatcher = (watchPath: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be worth extracting this into a util

Copy link
Author

Choose a reason for hiding this comment

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

Ill see. It might become more obvious when we add another subscription later. Ill look into it.

@Context('environment') environment: Environment,
): Promise<AsyncIterator<unknown, any, undefined>> {
if (!environmentWatchers.get(environment.id)) {
const watcher = setWatcher(path.join(environment.dirPaths.dbmssData, '**', 'run', '*.pid'));
Copy link
Author

Choose a reason for hiding this comment

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

will assign this path to a variable somewhere better at the end.

@jk05 jk05 force-pushed the dbmss-info-subscription branch 4 times, most recently from cdfd177 to 7ae5d4b Compare October 5, 2021 13:48
@jk05 jk05 requested a review from nglgzz October 5, 2021 13:51
@jk05 jk05 removed the help wanted Extra attention is needed label Oct 5, 2021
@jk05 jk05 marked this pull request as ready for review October 5, 2021 13:53
@jk05 jk05 force-pushed the dbmss-info-subscription branch 2 times, most recently from d49b8ef to 76efa2e Compare October 5, 2021 14:20
Copy link
Contributor

@nglgzz nglgzz left a comment

Choose a reason for hiding this comment

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

Looks good ✨

@jk05 jk05 merged commit 21d945e into neo4j-devtools:master Oct 5, 2021
@jk05 jk05 deleted the dbmss-info-subscription branch October 5, 2021 15:22
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.

2 participants