[STUD-490] add cluster api support by rmyers · Pull Request #236 · stardog-union/stardog.js

rmyers · 2020-10-12T21:46:24Z

Adding support for stardog cluster api calls:

GET /admin/cluster - cluster info
GET /admin/cluster/status - cluster status

I have added some tests that mock the cluster responses as it seems that setting up a cluster for testing will require a lot of setup. I think this can be done with a new docker image or setup the existing one to create a cluster.

I based this off of the 'fix tests for 7' branch so probably easiest to look at the last commit for reviewing this.

jmrog · 2020-10-13T14:38:50Z

@rmyers For the purposes of cleaner review, can you rebase commit a609373 on the v2 branch when you have a chance? The "fix tests" branch has been merged into v2 now. Thanks!

jmrog

Looks good, but a few requests for changes. Most of them are inline, but here's an additional one: you'll need to add type information for the cluster calls to "index.d.ts".

jmrog · 2020-10-13T14:44:29Z

+module.exports = {
+  info,
+  status,
+};


Looks good, but a few general (somewhat nitpicky) comments here:

Instead of requiring Fetch and doing Fetch.fetch, the convention in stardog.js is to do const { fetch } = require('./fetch'). Arguably, the former is cleaner because it doesn't shadow the fetch global variable in the browser and makes it explicit that you are calling a different fetch, but this is a small matter and I'd prefer sticking to convention for now.

You can leave out the params parameter when it's not being used. Here, the stardog.js codebase is simply inconsistent right now -- sometimes the params parameter is included when it's not being used, and sometimes it isn't -- but we're trying to move toward "only include it when it's actually being used."

I changed the import to make it possible to mock the fetch function. Since it needs an Object to spyOn, couldn't get it to work with a direct import. But I'm working on getting the cluster to run with docker compose and then I can change the tests to a full integration instead of mocks to address this.

I was able to get a simple cluster to run and have updated the tests to use the cluster nodes for the connection without mocking fetch. This should address the code style issues.

jmrog · 2020-10-13T15:05:33Z

+      }
+    );
+
+    done();


We try to avoid having to call done manually in the stardog.js tests (mostly just another convention, but it does help a little bit to avoid the "oops, I forgot" case, and it also removes the need to write your own catch clause). With promises, Jest can handle this itself. You could do:

const clusterPromise = cluster.info(conn); expect(Fetch.fetch).toHaveBeenCalledTimes(1); /* . . . etc . . . */ return clusterPromise.then(res => { /* . . . etc . . . */ });

jmrog · 2020-10-13T15:14:49Z

+      }
+    );
+
+    done();


Same comment here as on the other file.

jmrog · 2020-10-13T15:15:16Z

+  // TODO: This case is currently skipped because there is no way in Stardog
+  // 7.3.x to ensure that the DB is ready for optimization (so, this often
+  // errors out). The erroring out should be fixed in Stardog 7.4.1.
+  it.skip('should optimize an online DB', () =>


This part will need to be removed from the PR, as it's stale.

rmyers · 2020-10-14T14:58:23Z

@jmrog I have added a sample docker-compose and addressed the pr comments. When you have time please have a look. I'm not exactly sure how to get this going in circleci as I'm guessing that would break all the pr's and branches that don't have this code. You can however run this locally to test it out:

docker-compose -f .circleci/docker-compose.yml up

then in another terminal run:

npm run test:cluster

jmrog

This looks great! I made a nitpicky comment inline that I'd like to have addressed before merging, but otherwise seems ready to go. (I can handle updating CircleCI-related stuff as necessary.)

jmrog · 2020-10-21T13:26:26Z

+      const coordinator = nodes.find(node => node.search(/5821/) > 0);
+      const participant = nodes.find(node => node.search(/5822/) > 0);


Nitpick-ish: Instead of node.search(regex) > 0, you can just more directly do node.includes(number) here to accomplish the same purpose a little more clearly.

jmrog · 2020-10-21T13:26:44Z

+      const coordinator = nodes.find(node => node.address.search(/5821/) > 0);
+      const participant = nodes.find(node => node.address.search(/5822/) > 0);


Same comment here (and below) re: includes.

jmrog

LGTM

This reverts commit ac16734.

* Feature/154 upgrade fetch ponyfill (#155) * Upgrade fetch ponyfill * Get tests passing again after upgrading fetch-ponyfill * Add support, tests, and typings for additionalHandlers on query execute (#157) * Update package-lock.json * Fix docs script, update package-lock * Update CHANGELOG.md for v1.2.1, add tag * Add release documentation, as well as annoying prepublish alert * Upgrade typedocs to resolve marked security vulnerability (fix #158) * 1.2.2 * Update package-lock * Make additionalHandlers optional * Add method for retrieving server status info (close #165) (#166) * Fix package-lock.json for v2 * Add ICV report functionality and fix existing types (#188) * Update to version 6 and fix CircleCI (#185) * add full string values for test expectations * fix tests and update version note in README for stardog 6 * attempt to fix circleci by updating to new jfrog url * explicitly test graphql explain support (#184) * Add icv.report and icv.reportInTransaction functionality (close #187) * Add types for new ICV methods and fix types for existing (close #186) * Add tests for new ICV methods * add json query explain support (#191) * Try to fix dependencies * Fix tests * add stored query update (PUT) method * handle stored query updates when version < 6.2.0 * cleanup poc of server version helper * instead of getting the version recover from the method not supported error * cleanup * cleanup * code review feedback * add support for `reasoning` and `description` attributes to stored queries (#196) * add two endpoints for getting all db properties default values and getting all actual values and upgrade stardog to latest (#198) * Add ability to import namespaces via string or file (new Stardog endpoints) (#201) * Add ability to import namespaces via string or file (new Stardog endpoints) * Use Object.assign instead of spread syntax * Small tweak * Docs/type updates and name change * Docs change * Update generated docs * Allow `onResponseStart` to abort further stardog.js processing (#203) * Allow onResponseStart to abort any further stardog.js processing * Add test case, plus lint fixes * Better docs, too * use params in graphql query (#202) * Add ability to send properties file on db.create (#205) * Make old-style namespaces request really request only namespaces (fix #207) (#208) * Add support for streaming export data to file (and compression) (#212) * Add support for streaming data export, and for compression * Add tests for additionalHandlers with exportData * Add format flag for exportData * Add ability to import file to VG (for CSV/JSON import) (#213) * Add support for importing from file to VG (close #209) * Tests and fixtures * Fix up tests * Update README and test name * Fix types for onResponseStart * Make input_file come last * Support updating GraphQL schemas without remove/add (with backwards compat) (#215) * Add reasoning flag to example * add db model endpoint (#224) * Fix db.model type and docs * add virtual graph list info (#223) * Add support for database constraint for virtual graphs (#225) * Handle commented-out prefix lines in queries (fix #228) (#229) * Add additionalHandlers to graphql.execute (#231) * Change content-type to use utf-8 (#233) * [STUD-414] Make all tests work with Stardog 7+ (#234) * Remove db.copy command and test * Fix graphql tests * Fix graphStore tests * Skip versioning tests for now * Fix ICV tests * Fix exportDB tests * Fix createDB and optimizeDB tests * Fix query tests * Update version support info in README * Update test for Stardog 7.4.1 * Quick test updates * [STUD-490] add cluster api support (#236) * [STUD-490] Add cluster api support * [STUD-490] Removing done callback in spec files * [STUD-490] Removing unused param and add docs * [STUD-490] Adding docker-compose cluster test suite * [STUD-490] feature: add cluster support - address comments * Test adjustment for Stardog 7.4.2+ * Make tests more flexible * Remove DB copy and versioning * Adjust a few tests * update versions matrix * Update docs * [STUD-528] Add Data Sources (#239) * [STUD-528] add data sources * rename options to opts * return res * [STUD-517] data hub - allow stardog.js to accept query params (#241) * [STUD-517] feature: data hub - allow stardog.js to accept query params for data sources remove api * [STUD-517] feature: data hub - pr suggestion, add param to docs, run docs script, append query parameters * [STUD-517] feature: data hub - update docs, change name string to template * Add VG online + data source metadata (#240) * add vg online * fix docs * add data source to vg meta * always send data_source * no snakes * [STUD-XXX] Add dataSource.share (#249) * doc * share * [STUD-XXX] Add force option to dataSource.update (#248) * Add force param to dataSource.update * Params to request options, don't always include force in body * Fix doc for vg.mappings * Update doc for dataSource.update * [STUD-559] user tests - fix expected res in user tests (#247) * Add custom changelog generation * README update * Update Stardog version info; temporarily disable reasoning tests * Make some tests rely less on exact serialization from Stardog Co-authored-by: Brian J. Rubinton <brianrubinton@gmail.com> Co-authored-by: Annee Barrett <ahuang852@gmail.com> Co-authored-by: Josh Kim <joshhk72@gmail.com> Co-authored-by: Robert Myers <robert.myers@stardog.com> Co-authored-by: Carl Chaffatt <carl.chaffatt@stardog.com> Co-authored-by: Josh Kim <josh.kim@stardog.com>

jmrog suggested changes Oct 13, 2020

View reviewed changes

jmrog suggested changes Oct 21, 2020

View reviewed changes

rmyers added 5 commits October 21, 2020 12:10

[STUD-490] Add cluster api support

2ce9d2a

[STUD-490] Removing done callback in spec files

6853cc7

[STUD-490] Removing unused param and add docs

5197075

[STUD-490] Adding docker-compose cluster test suite

af8c880

[STUD-490] feature: add cluster support - address comments

66fd329

jmrog approved these changes Oct 21, 2020

View reviewed changes

jmrog merged commit ac16734 into stardog-union:v2 Oct 21, 2020

rmyers deleted the feature/STUD-490-add-cluster-api branch October 21, 2020 17:21

jmrog pushed a commit that referenced this pull request Oct 22, 2020

Revert "[STUD-490] add cluster api support (#236)"

7bcd53a

This reverts commit ac16734.

jmrog mentioned this pull request Feb 22, 2021

Add cluster API support #251

Closed

+                    }
+                  );
+                  done();

		const coordinator = nodes.find(node => node.search(/5821/) > 0);
		const participant = nodes.find(node => node.search(/5822/) > 0);

Conversation

rmyers commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmrog commented Oct 13, 2020

Uh oh!

jmrog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmyers commented Oct 14, 2020

Uh oh!

jmrog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmrog left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rmyers commented Oct 12, 2020 •

edited

Loading