You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2020/02/04 08:05:03 UTC
[GitHub] [tinkerpop] heljoyLiu opened a new pull request #1243: js: add
session connection
heljoyLiu opened a new pull request #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] heljoyLiu closed pull request #1243: js: add session
connection
Posted by GitBox <gi...@apache.org>.
heljoyLiu closed pull request #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] jorgebay commented on a change in pull request #1243:
js: add session connection
Posted by GitBox <gi...@apache.org>.
jorgebay commented on a change in pull request #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#discussion_r380120136
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js
##########
@@ -165,6 +169,21 @@ class Connection extends EventEmitter {
};
}
+ if (this._session === true && op !== 'authentication') {
+ if (bytecode !== null) {
+ reject(new Error('do not support bytecode in session mode'));
+ return;
+ }
+ if (args === null) {
+ reject(new Error('get nil args in request'));
+ return;
+ }
+
+ const message = Buffer.from(this._header + JSON.stringify(this._getSessionRequest(requestId, null, args)))
Review comment:
Missing `;` at the end of the statement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] jorgebay commented on issue #1243: js: add session
connection
Posted by GitBox <gi...@apache.org>.
jorgebay commented on issue #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#issuecomment-585202966
Thanks @heljoyLiu for the patch.
Sorry I didn't have time to review it yet, I'll do it this week.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] heljoyLiu commented on issue #1243: js: add session
connection
Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#issuecomment-588130220
> > are you ok with this only going to master branch? should it be aimed at a current release branch instead?
>
> Thanks for pointing it out.
>
> Would it be possible to base your PR onto `3.3-dev`, @heljoyLiu ?
> Otherwise, I can do it myself after review is finished.
I make one new PR on `3.3-dev`, thanks @jorgebay
https://github.com/apache/tinkerpop/pull/1251
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] heljoyLiu commented on a change in pull request #1243:
js: add session connection
Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on a change in pull request #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#discussion_r380636902
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js
##########
@@ -95,11 +95,15 @@ class Connection extends EventEmitter {
this.isOpen = false;
this.traversalSource = options.traversalSource || 'g';
this._authenticator = options.authenticator;
+ this._session = options.session || false;
Review comment:
ok, thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] jorgebay commented on a change in pull request #1243:
js: add session connection
Posted by GitBox <gi...@apache.org>.
jorgebay commented on a change in pull request #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#discussion_r380123784
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js
##########
@@ -199,6 +218,22 @@ class Connection extends EventEmitter {
});
}
+ _getSessionRequest(id, op, args) {
+ if (args) {
+ args = this._adaptArgs(args, true);
+ } else {
+ args = {'gremlin': 'session.close()'};
+ }
+ args['session'] = this._sessionId;
+
+ return ({
+ 'requestId': { '@type': 'g:UUID', '@value': id },
+ 'op': op || 'eval',
+ 'processor': 'session',
+ 'args': args
+ });
+ }
Review comment:
Maybe we could extract common functionality between `_getRequest()` and `_getSessionRequest()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] jorgebay commented on issue #1243: js: add session
connection
Posted by GitBox <gi...@apache.org>.
jorgebay commented on issue #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#issuecomment-588131157
Thanks @heljoyLiu , I'll review it shortly.
Feel free to close this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] jorgebay commented on a change in pull request #1243:
js: add session connection
Posted by GitBox <gi...@apache.org>.
jorgebay commented on a change in pull request #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#discussion_r380125983
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js
##########
@@ -165,6 +169,21 @@ class Connection extends EventEmitter {
};
}
+ if (this._session === true && op !== 'authentication') {
+ if (bytecode !== null) {
+ reject(new Error('do not support bytecode in session mode'));
+ return;
+ }
+ if (args === null) {
+ reject(new Error('get nil args in request'));
+ return;
+ }
Review comment:
This should be extracted at the beginning of the method:
```javascript
submit(bytecode, op, args, requestId, processor) {
if (bytecode !== null) {
return Promise.reject(new Error('Bytecode in session mode is not supported'));
}
if (args === null) {
return Promise.reject(new Error('args should not be null'));
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1243: js: add session
connection
Posted by GitBox <gi...@apache.org>.
spmallette commented on issue #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#issuecomment-587419589
As a quick note @heljoyLiu given that we are trying to quickly turn around a 3.4.6 release to fix a bug in 3.4.5:
https://lists.apache.org/thread.html/r7de7d160173d0774720b347122409bb6e46268613205b08edaa687ee%40%3Cdev.tinkerpop.apache.org%3E
it may take some extra time to see this change merged.
@jorgebay are you ok with this only going to master branch? should it be aimed at a current release branch instead?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] heljoyLiu commented on a change in pull request #1243:
js: add session connection
Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on a change in pull request #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#discussion_r380639860
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js
##########
@@ -165,6 +169,21 @@ class Connection extends EventEmitter {
};
}
+ if (this._session === true && op !== 'authentication') {
+ if (bytecode !== null) {
+ reject(new Error('do not support bytecode in session mode'));
+ return;
+ }
+ if (args === null) {
+ reject(new Error('get nil args in request'));
+ return;
+ }
+
+ const message = Buffer.from(this._header + JSON.stringify(this._getSessionRequest(requestId, null, args)))
Review comment:
sorry for this obvious mistake
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] jorgebay commented on a change in pull request #1243:
js: add session connection
Posted by GitBox <gi...@apache.org>.
jorgebay commented on a change in pull request #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#discussion_r380117937
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js
##########
@@ -95,11 +95,15 @@ class Connection extends EventEmitter {
this.isOpen = false;
this.traversalSource = options.traversalSource || 'g';
this._authenticator = options.authenticator;
+ this._session = options.session || false;
Review comment:
We should add `options.session` to the `Connection` and to the `Client` jsdoc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] jorgebay commented on a change in pull request #1243:
js: add session connection
Posted by GitBox <gi...@apache.org>.
jorgebay commented on a change in pull request #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#discussion_r380121270
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js
##########
@@ -365,6 +400,11 @@ class Connection extends EventEmitter {
}
if (!this._closePromise) {
this._closePromise = new Promise(resolve => {
+ if (this._session === true) {
+ const message = Buffer.from(this._header + JSON.stringify(this._getSessionRequest(utils.getUuid(), 'close', null)))
Review comment:
Missing `;`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] heljoyLiu commented on issue #1243: js: add session
connection
Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#issuecomment-587437098
> > are you ok with this only going to master branch? should it be aimed at a current release branch instead?
>
> Thanks for pointing it out.
>
> Would it be possible to base your PR onto `3.3-dev`, @heljoyLiu ?
> Otherwise, I can do it myself after review is finished.
ok, Thanks. I will make a new PR with this suggestion on '3.3-dev'
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tinkerpop] jorgebay commented on issue #1243: js: add session
connection
Posted by GitBox <gi...@apache.org>.
jorgebay commented on issue #1243: js: add session connection
URL: https://github.com/apache/tinkerpop/pull/1243#issuecomment-587420470
> are you ok with this only going to master branch? should it be aimed at a current release branch instead?
Thanks for pointing it out.
Would it be possible to base your PR onto `3.3-dev`, @heljoyLiu ?
Otherwise, I can do it myself after review is finished.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services