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

Adding flock to JS examples #85

Merged
merged 7 commits into from Oct 4, 2019

Conversation

@prashant-shahi
Copy link
Member

commented Oct 3, 2019

This example app follows Flock using Go closely.


This change is Reviewable

- Included flock js clone in examples
- Added credentials-template.json as a template for crendentials.json
- Added package.json file
- Adding Dockerfile for the flock node app
- Adding docker-compose yaml file for Dgraph Cluster
- Adding docker-compose-flock yaml file for flock node app
- Updating package.json file to rebuild
- Replacing Done message with report status notice when success
Copy link

left a comment

A review job has been created and sent to the PullRequest network.


Check the status or cancel PullRequest code review here.

Copy link
Member

left a comment

Reviewed 1 of 9 files at r1.
Reviewable status: 1 of 9 files reviewed, 4 unresolved discussions (waiting on @danielmai, @mangalaman93, @paulftw, and @prashant-shahi)


examples/flock/Dockerfile, line 4 at r1 (raw file):

FROM node:lts-alpine

LABEL maintainer="Prashant Shahi<prashant@dgraph.io>"

Add a space between the name and email: Prashant Shahi <prashant@dgraph.io>.


examples/flock/docker-compose.yml, line 15 at r1 (raw file):

        source: $DATA_DIR
        target: /dgraph
    restart: on-failure

Consider removing these restart: on-failure configs, so we can catch issues of Zero stopping.


examples/flock/docker-compose.yml, line 29 at r1 (raw file):

        source: $DATA_DIR
        target: /dgraph
    restart: on-failure

Consider removing these restart: on-failure configs, so we can catch issues of Alpha stopping.


examples/flock/docker-compose-flock.yml, line 11 at r1 (raw file):

      - ALPHA_ADDR=localhost:9080
      - LOG_INTERVAL_TIME=2000
    restart: always

Consider removing these restart: on-failure configs, so we can catch issues of Zero or Alpha stopping.

- Added .dockerignore file
- Updated Dockerfile
- Updated package.json file
@prashant-shahi prashant-shahi requested a review from danielmai Oct 3, 2019
Copy link
Member Author

left a comment

Reviewable status: 0 of 10 files reviewed, 4 unresolved discussions (waiting on @danielmai and @paulftw)


examples/flock/Dockerfile, line 4 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Add a space between the name and email: Prashant Shahi <prashant@dgraph.io>.

Done.


examples/flock/docker-compose.yml, line 15 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Consider removing these restart: on-failure configs, so we can catch issues of Zero stopping.

Done.


examples/flock/docker-compose.yml, line 29 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Consider removing these restart: on-failure configs, so we can catch issues of Alpha stopping.

Done.


examples/flock/docker-compose-flock.yml, line 11 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Consider removing these restart: on-failure configs, so we can catch issues of Zero or Alpha stopping.

Done.

@prashant-shahi prashant-shahi merged commit 5603c34 into master Oct 4, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 10 files, 4 discussions left (danielmai, paulftw)
Details
Build (dgraph-js) TeamCity build finished
Details
license/cla Contributor License Agreement is signed.
Details
@prashant-shahi prashant-shahi deleted the prashant/adding-flock branch Oct 4, 2019
const query = [];

query.push(`t as var(func: eq(id_str, "${tweet.id_str}"))`);
query.push(`u as var(func: eq(user_id, "${tweet.author.user_id}"))`);
Comment for lines +146  – +149

This comment has been minimized.

Copy link
@paulftw

paulftw Oct 4, 2019

Contributor
  const query = [
    `t as var(...)`,
    `u as var(...)`,
  ];
const finalQuery = `query {${query.join('\n')}}`;
return finalQuery;
Comment for lines +164  – +165

This comment has been minimized.

Copy link
@paulftw

paulftw Oct 4, 2019

Contributor
  return `query {...}`;
async function main() {
await setSchema();
setInterval(reportStats, LOG_INTERVAL_TIME);
client.stream('statuses/sample.json', function(stream) {

This comment has been minimized.

Copy link
@paulftw

paulftw Oct 4, 2019

Contributor

client is instantiated at the top of the file but only used once here. I think const client=... should be inside main

async function wait(time) {
return new Promise((resolve) => {
const id = setTimeout(
() => {
clearTimeout(id);
resolve();
},
time,
);
});
}

Comment for lines +176  – +187

This comment has been minimized.

Copy link
@paulftw

paulftw Oct 4, 2019

Contributor

Since this is test code const wait = time => new Promise(resolve => setTimeout(resolve, time)) is good enough and shorter. const id isn't used anyway so no reason to keep it around or call clearTimeout

}

function reportStats() {
const now = new Date().getTime();

This comment has been minimized.

Copy link
@paulftw

paulftw Oct 4, 2019

Contributor
paul client 𝝺 node
Welcome to Node.js v12.10.0.
Type ".help" for more information.
> new Date().getTime()
1570204761045
> Date.now()
1570204765965

Date.now() returns same value but shorter to type

@paulftw

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

I left a few minor comments, but overall I would expect the Flock example to be in its own repo for several reasons:

  • it's pretty big and somewhat complicated compared to other examples
  • it's interesting and useful on its own, not just a mere example inside the Dgraph JS SDK
  • there's no automatic script to make sure flock in this repo will remain compatible with dgraph-js in this same repo. Without it flock can eventually become out-of-date and cause a broken window syndrome.
  • PRs to Flock may be more frequent than PRs to the client libraries. Separate github repo would give us a separate review queue and a separate commit log, both would be helpful to speed up updates to the client lib (arguably more important piece of code).

If you guys feel strongly about keeping it here I'm ok with that.
I'd simply add a link to the flock repo into README here, or have a single line "see this repository ..." in the examples/flock/README

@prashant-shahi

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2019

After reading the reasons, I think it makes more sense to separate the flock JS example repo from the client repositories.

Copy link
Member

left a comment

Maybe we should move all of the Flock implementations into a single repo, that way the architecture is documented only in a single place and the code is divided by language.

Sounds like a good plan, I agree with @paulftw

On the other hand, I expected this code to be thoroughly commented, as we discussed.
The goal of this code is not to test Dgraph but to serve as a teaching tool.
Someone that has never used Dgraph should be able to start reading the code and understand what's going on.

Also, does this say this PR is already merged?

Reviewed 1 of 9 files at r1, 1 of 5 files at r2.
Reviewable status: 2 of 10 files reviewed, 11 unresolved discussions (waiting on @danielmai and @prashant-shahi)


examples/flock/index.js, line 12 at r2 (raw file):

const startStatus = new Date().getTime();

let lastStatus = 0;

I do not see where lastStatus is used


examples/flock/index.js, line 14 at r2 (raw file):

let lastStatus = 0;
let retry = true;
let failureCount = 0;

what's the difference between failureCount and errorCount?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.