update with new versioning #228

ttvi · 2017-02-27T19:17:39Z

What changed:

update listing algorithms delimiterMaster and delimiterVersions to reflect new versioning in metadata (with metadata's functional tests to verify)
remove unused constants
change versioning tools to version class
update unit tests

ghost · 2017-02-27T19:44:15Z

What about S3 functional tests ? The listing algorithm is used for the file backend.

ttvi · 2017-02-27T20:35:21Z

There should be no impact. The listing algorithms delimiterMaster and delimiterVersions are specific to versioning only.

MathieuCassagne · 2017-03-21T15:23:00Z

lib/algos/list/Extension.js

+     *                    < 0: entry is not accepted, listing should finish
+     */
+    filter(entry) {
+        return entry ? 0 : 0;


You always return 0 ?

This is the base class.

MathieuCassagne · 2017-03-21T15:28:06Z

lib/algos/list/MPU.js

@@ -3,6 +3,11 @@
 const checkLimit = require('./tools').checkLimit;
 const DEFAULT_MAX_KEYS = 1000;

+function inc(str) {


This function is present in 3 files, can you make it into a separate file ?

MathieuCassagne · 2017-03-21T15:31:22Z

lib/algos/list/basic.js

        this.res = [];
        if (parameters) {
-            this.maxKeys = checkLimit(parameters.maxKeys, DEFAULT_MAX_KEYS);
+            const maxKeys = parameters.maxKeys;


Why do this in two steps ?

MathieuCassagne · 2017-03-21T15:37:12Z

lib/algos/list/Extension.js

+
+    /**
+     * Get the listing resutls. Format depends on derivatives' specific logic.
+     * @return {Array} - The listed elements


this.res is always undefined.

MathieuCassagne · 2017-03-21T15:39:23Z

lib/algos/list/basic.js

@@ -34,11 +54,11 @@ class List {
    filter(elem) {


You return a Number now.

Thanks. Updated.

MathieuCassagne · 2017-03-21T15:40:03Z

lib/algos/list/delimiter.js

@@ -94,30 +127,12 @@ class Delimiter {
     */
    addContents(key, value) {


Same about the return.

rahulreddy · 2017-03-21T17:55:38Z

lib/algos/list/MPU.js

@@ -3,6 +3,11 @@
 const checkLimit = require('./tools').checkLimit;
 const DEFAULT_MAX_KEYS = 1000;

+function inc(str) {


@ttvi You can add this method in lib/algos/list/tools.js as a utility function and import here. Please add a short jsdoc description on top for the function.

rahulreddy · 2017-03-21T18:02:54Z

lib/algos/list/MPU.js

+    genMDParams() {
+        const params = {};
+        if (this.params.keyMarker) {
+            params.gt = `overview${this.params.splitter}` +


Please abstract the key prefix generation into a method.

We only use it once, I don't see the need for doing that. It's better to keep the code as is to see what it does.

Let's keep it as-is.

rahulreddy · 2017-03-21T19:50:35Z

lib/algos/list/basic.js

 const checkLimit = require('./tools').checkLimit;
 const DEFAULT_MAX_KEYS = 10000;

 /**
 *  Class of an extension doing the simple listing
 */
-class List {
+class List extends Extension {


~~extends~~ super ~~currently performs poor in pure ES6 implementation. You can choose to use the class directly instead or introduce base class in a PR later when performance regression is fixed in node 6.~~

Source: https://kpdecker.github.io/six-speed/ for reference.

isn't that part of the "class" tests ? They report "same" as I see it, or I may not be looking at the right row ?
EDIT: Or maybe we're talking about super which is yet again different ? If so, I'm not sure that's a real issue here.

Is this still an issue here considering the time nodejs spends to do "super" compared to the listing process itself? The number 4x is significant but in the context of listing I do not really see that as an issue.
Definitely that needs to be improved though. What is your suggestion here: removing super and duplicating the code or is there any cool approach you can think of?

I don't think it's an issue. The benchmark compares to the ES5 implementation, so it's 4x slower than the ES5 implementation but fast enough for us since the code would spend nowhere near time number of times making as many super calls as the benchmark.

Let's keep it as-is. @rahulreddy let me know if you want to open an issue for this.

ghost

Lots of good things in there, + a few questions about doc, explanations/clarifications, and design improvement suggestions

ghost · 2017-03-21T23:12:50Z

lib/algos/list/Extension.js

+'use strict'; // eslint-disable-line strict
+
+/**
+ *  Base class of listing extensions.


I personally like this :D

ghost · 2017-03-22T09:21:03Z

lib/algos/list/Extension.js

+'use strict'; // eslint-disable-line strict
+
+/**
+ *  Base class of listing extensions.


👍 for this abstraction to clarify things :)

ghost · 2017-03-22T09:21:43Z

lib/algos/list/MPU.js

@@ -3,6 +3,11 @@
 const checkLimit = require('./tools').checkLimit;
 const DEFAULT_MAX_KEYS = 1000;

+function inc(str) {


ghost · 2017-03-22T09:32:48Z

lib/algos/list/Extension.js

+     *
+     * @return {string} - the insight; a common prefix or a master key
+     */
+    skipping() {


You might want to return a predefined constant here.
For instance:

const SKIP_REASON_NONE = ''; const SKIP_REASON_WHATEVER = 'whatever'; ... class Extension { ... skipping() { return SKIP_REASON_NONE; }

This scheme could be reused by future extensions to only return known and checkable reasons.

ghost · 2017-03-22T09:50:49Z

lib/algos/list/delimiter.js

-                creationDate: tmp.creationDate,
-            },
-        });
+        this.Contents.push({ key, value });


Just to double-check, why are we changing this whole bit ? Is it because value is now expected to contain versionning information + the former Object within ?

Does it have an impact on MD or S3 ? or Both ?

IF it is both, it's becoming a REAL issue, as we won't be able to make progress until the whole work is ready...

Also, where is this formatting done now ?

To make S3 to do the computation instead of the leader node. S3 needs to do the parsing scality/cloudserver@4415788#diff-d1fea59d8501a89ee1ce02464ecdcd16; no metadata impact.
To make them consistent across extensions (w.r.t Basic and Versions originally).

ghost · 2017-03-22T10:42:33Z

lib/versioning/Version.js

+     * @return {boolean} - whether this is a PHD version
+     */
+    static isPHD(value) {
+        // check the length of the value before parsing


Check the length ? but we're only checking if the value contains "isPHD" before actually parsing the value for a real check -> mismatch between code and comment

That's the description for the previous version. Will be removed.

ghost · 2017-03-22T10:45:12Z

lib/versioning/Version.js

+        try {
+            return Version.from(value).isPHDVersion();
+        } catch (exception) { // eslint-disable-line strict
+            return false; // nice, Vault


huh ? Vault ?

Vault is sending plain text data not in JSON format. Vault should never contain "isPHD" in the data it sends, but if it does, we have this covered.

ok, nice defensive coding then :)

ghost · 2017-03-22T12:44:19Z

lib/versioning/VersionID.js

+ * @return {string} - the formated versionId string
+ */
+function generateVersionId(info) {
+    // Need to wait for the millisecond slot got "flushed". We wait for


I'm not sure to understand this bit. I get that you wait for one millisecond, but not why ?
Is it because you need to ensure that it starts fresh on a "new" millisecond, due to the inner workings relying on millisecond + sequence number ? Are you relying on the fact that Node.JS computation is monothreaded + this mechanism, to make sure that any two instantiations of this class cannot start on the same millisecond ?
If yes, please fix doc to reflect this.

Epochs are at millisecond level. Wait one millisecond to make sure the first epoch when this module started is always the latest, regardless of the sequence number.

ghost · 2017-03-22T12:51:46Z

lib/versioning/VersionID.js

+ * @param {string} str - the versionId to encrypt
+ * @return {string} - the encrypted versionId
+ */
+function encrypt(str) {


the encrypt/decrypt part is not used for MD.
It should have gone to another PR, depending on this one.

No matter it's too late, but please keep this in mind, as such huge reviews are a hassle for everyone of us.

Arsenal is in the middle to provide common knowledge to both S3 and MetaData. Since it's related to versioning, it belongs to this specific PR.

ghost · 2017-03-22T12:55:16Z

tests/unit/algos/list/delimiterMaster.js

@@ -1,179 +0,0 @@
-'use strict'; // eslint-disable-line strict


Why have no tests for this one ?

It has been merged with the tests for delimiter.

ghost · 2017-03-24T14:58:34Z

LGTM now, Good work o/

lamphamsy · 2017-03-24T15:36:14Z

lib/algos/list/basic.js

@@ -34,11 +54,11 @@ class List {
    filter(elem) {


lamphamsy · 2017-03-24T16:03:57Z

lib/versioning/VersionID.js

+const TEMPLATE_ST = new Array(LENGTH_ST + 1).join(' ');
+
+// site identifier, like PARIS, TOKYO; will be trimmed if exceeding max length
+const SITE_ID = `${process.env.SITE_ID}${TEMPLATE_ST}`.slice(0, LENGTH_ST);


Could process.env.SITE_ID be undefined? If yes, should it be process.env.SITE_ID || ${a_default_name}?

No. SITE_ID is a new required variable to be available to both S3 and MetaData https://github.com/scality/Federation/pull/937.

lamphamsy · 2017-03-24T16:17:39Z

lib/versioning/VersionID.js

+            VID_CRYPTO_ENCODING_TXT, VID_CRYPTO_ENCODING);
+    encrypted += cipher.final(VID_CRYPTO_ENCODING);
+    let length = encrypted.length;
+    while (length && encrypted.charAt(length - 1) === '=') {


Why '=' must be removed?

To beautify the base64 versionId string without impacting the correctness.

A regexp could be used to make things slightly clearer than a loop IMO, say, this should be equivalent (considering that base64 reserves = only for the trailer):

encrypted = encrypted.replace(/=/g,'')

Yes it was not carefully crafted. Some other alternatives can be used like a simple indexOf, but regex seems concise and is a good solution.

ttvi · 2017-03-24T21:45:51Z

Integration with versioning OK: http://ci.ironmann.io/gh/scality/Integration/11082.
Trying integration without versioning: http://ci.ironmann.io/gh/scality/Integration/11089.

ttvi · 2017-03-25T00:26:39Z

Ah, I just realized that this has completely changed the format of listing so this branch of Arsenal cannot be tested against the non-versioning branch of the other repos. @LaurenSpiegel @rahulreddy please merge this all together with S3's scality/cloudserver#651 and MetaData's https://github.com/scality/MetaData/pull/1009.

jonathan-gramain · 2017-03-31T17:50:35Z

lib/algos/list/Extension.js

+     *                    < 0: entry is not accepted, listing should finish
+     */
+    filter(entry) {
+        return entry ? FILTER_SKIP : FILTER_SKIP;


If I understand correctly, this is considered an abstract base class with default implementations of some methods. Here it looks like you simply want to return FILTER_SKIP. Or does it make more sense to return FILTER_ACCEPT for a default implementation, which would be no filtering at all? Or maybe just remove the default implementation if it makes no sense for subclasses.

I agree this does not look nice. As this is an interface, I wanted to give some specific description about each function can so that we can get the general idea of the derivatives. About the double FILTER_SKIPs there, they exist for some purposes: first, to give an example on what to return (matching what described in JSDoc), and second, to avoid the linting warning of not using the input value entry (while I still want to keep the entry there).

jonathan-gramain · 2017-03-31T17:53:24Z

lib/algos/list/Extension.js

+     * version when doing master version listing.
+     *
+     * @return {string} - the insight: a common prefix or a master key,
+     *                                 or undefined if there is no insight


It seems the comment should be updated, since the function returns SKIP_NONE.

jonathan-gramain · 2017-03-31T18:07:39Z

lib/algos/list/MPU.js

+            params.gt = `overview${this.params.splitter}` +
+                `${this.params.keyMarker}${this.params.splitter}`;
+            if (this.params.uploadIdMarker) {
+                params.gt += `${this.params.uploadIdMarker}`;


You probably don't need the formatting markers here

I don't know if this.params.uploadIdMarker could be a number. As with the param.gt, I copied it from the old code.

I think the + operator on a string automatically converts its second argument into a string.

jonathan-gramain · 2017-03-31T18:14:37Z

lib/algos/list/MPU.js

+            }
+            // advance so that lower bound does not include the supplied
+            // markers
+            params.gt = inc(params.gt);


Don't you want to start the listing from the marker instead of the "incremented" marker? (and gt would not include the marker itself unlike gte).

I've got them all from the old code https://github.com/scality/MetaData/pull/1009/files#diff-b596dc6bd6d51b111a52033729463bd1L189. Maybe @LaurenSpiegel knows what it does.

jonathan-gramain · 2017-03-31T18:20:43Z

lib/algos/list/MPU.js

     */
    filter(obj) {
        // Check first in case of maxkeys = 0
        if (this.keys >= this.maxKeys) {
            // In cases of maxKeys <= 0 => IsTruncated = false
            this.IsTruncated = this.maxKeys > 0;
-            return false;
+            return -1;


You should now use FILTER_END and family

jonathan-gramain · 2017-03-31T23:05:39Z

lib/versioning/VersionID.js

+        return diff[0] * 1e9 + diff[1];
+    }
+    const start = process.hrtime();
+    while (getspan(process.hrtime(start)) < span);


Would you mind putting the ; on the next line, or instead have an empty instruction block? So that it becomes clearer that it's a busy loop doing nothing.

jonathan-gramain · 2017-03-31T23:09:04Z

lib/versioning/VersionID.js

+const VID_CRYPTO_PASSWORD = process.env.VID_CRYPTO_PASSWORD || 'hangngon';
+
+// internal state of the module
+let prvts = 0; // epoch of the last versionId


I would opt for longer, more descriptive names for those variables, especially since they are global to the module.

jonathan-gramain · 2017-03-31T23:43:23Z

tests/unit/versioning/VersionID.js

+const VID = require('../../../lib/versioning/VersionID.js');
+const assert = require('assert');
+
+function randkey(length = 15) {


very minor: you may remove the default value for length and let the caller specify it explicitly each time, because I'm not sure what the default value is representing (i.e which kind of particular key) and if there is a meaningful default value here.

This is the comment I like; it touches the right issues + having convincing reasoning for its suggestions.

Ah, about the answer, it's there to represent the approximated length of a versionId string. I will move it to the caller.

jonathan-gramain · 2017-04-01T00:04:35Z

lib/versioning/VersionID.js

+            VID_CRYPTO_ENCODING_TXT, VID_CRYPTO_ENCODING);
+    encrypted += cipher.final(VID_CRYPTO_ENCODING);
+    let length = encrypted.length;
+    while (length && encrypted.charAt(length - 1) === '=') {


A regexp could be used to make things slightly clearer than a loop IMO, say, this should be equivalent (considering that base64 reserves = only for the trailer):

encrypted = encrypted.replace(/=/g,'')

jonathan-gramain · 2017-04-01T00:11:35Z

lib/versioning/VersionID.js

+const MAX_SQ = Math.pow(10, LENGTH_SQ) - 1; // good for 1 billion ops
+
+// the earliest versionId, used for versions before versioning
+const VID_INF = `${TEMPLATE_TS}${MAX_TS}`.slice(-LENGTH_TS) +


You could consider using helper functions to do the proper padding.

ttvi force-pushed the ft/vsp branch from 0009c38 to 8934c29 Compare February 28, 2017 09:15

ThibaultRiviere approved these changes Feb 28, 2017

View reviewed changes

ttvi force-pushed the ft/vsp branch 9 times, most recently from a2bb286 to ec68ccf Compare March 21, 2017 08:41

MathieuCassagne reviewed Mar 21, 2017

View reviewed changes

rahulreddy requested changes Mar 21, 2017

View reviewed changes

ghost self-assigned this Mar 22, 2017

ghost suggested changes Mar 22, 2017

View reviewed changes

ttvi force-pushed the ft/vsp branch 2 times, most recently from 90386ba to aaf31d7 Compare March 24, 2017 08:03

ghost approved these changes Mar 24, 2017

View reviewed changes

ThibaultRiviere approved these changes Mar 24, 2017

View reviewed changes

lamphamsy reviewed Mar 24, 2017

View reviewed changes

ttvi force-pushed the ft/vsp branch from ccf0bbb to 73a82a4 Compare March 24, 2017 18:16

ttvi force-pushed the ft/vsp branch from 73a82a4 to 08094ae Compare March 30, 2017 17:36

jonathan-gramain reviewed Apr 1, 2017

View reviewed changes

Vinh Tao added 3 commits April 1, 2017 16:23

rf: versioning constants and utilities

771ebb0

ft: versioning listing extensions

6187cde

test: versioning listing extensions

c9f007d

ttvi force-pushed the ft/vsp branch from 08094ae to b7efcba Compare April 1, 2017 14:26

electrachong force-pushed the ft/vsp branch from b7efcba to c9f007d Compare April 1, 2017 16:30

electrachong merged commit 149324b into master Apr 1, 2017

electrachong deleted the ft/vsp branch April 1, 2017 17:19

ttvi mentioned this pull request Apr 3, 2017

fix: wrong check of delimiter index #240

Merged

rahulreddy mentioned this pull request Apr 4, 2017

Tests for #240 #241

Closed

		@@ -94,30 +127,12 @@ class Delimiter {
		*/
		addContents(key, value) {

		@@ -1,179 +0,0 @@
		'use strict'; // eslint-disable-line strict

update with new versioning #228

update with new versioning #228

Conversation

ttvi commented Feb 27, 2017

ghost commented Feb 27, 2017

ttvi commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulreddy Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

ghost Mar 21, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttvi commented Mar 24, 2017

ttvi commented Mar 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulreddy Mar 21, 2017 •

edited

Loading

ghost Mar 21, 2017 •

edited by ghost

Loading