ft/ZENKO-229 get using location #1204

dora-korpar · 2018-04-12T17:44:51Z

Adds functionality to get object from specific location by passing header 'x-amz-location-constraint'

ironman-machine · 2018-04-12T17:49:22Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T01:49:40Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T04:15:31Z

PR has been updated. Reviewers, please be cautious.

rahulreddy · 2018-04-16T20:38:45Z

lib/api/apiUtils/object/locationConstraintCheck.js

+
+module.exports = {
+    locationConstraintCheck,
+    locationHeaderCheck,


I recommend separating it to it's own module to keep things modular.

rahulreddy · 2018-04-16T20:43:36Z

lib/api/apiUtils/object/locationConstraintCheck.js

+    const location = headers['x-amz-location-constraint'];
+    if (location) {
+        const locationConstraints = Object.keys(config.locationConstraints);
+        const validLocation = locationConstraints.includes(location);


This can just be

const validLocation = config.locationConstraints[location];

rahulreddy · 2018-04-16T20:45:15Z

lib/api/apiUtils/object/locationConstraintCheck.js

+        const locationConstraints = Object.keys(config.locationConstraints);
+        const validLocation = locationConstraints.includes(location);
+        if (!validLocation) {
+            return errors.InvalidArgument.customizeDescription(


Let's use errors.InvalidLocationConstraint since we already have it.

rahulreddy · 2018-04-16T20:46:36Z

lib/api/apiUtils/object/locationConstraintCheck.js

+        }
+        const locConfigObj = config.locationConstraints[location];
+        const bucketMatch = locConfigObj.details.bucketMatch;
+        let backendKey = objectKey;


Ternary operators make the logic look easy here

const backendKey = bucketMatch ? objectKey : `${bucketName}/${objectKey}`;

rahulreddy · 2018-04-16T20:59:19Z

lib/api/apiUtils/object/replicationBackendCompare.js

+    const locMatch = replicationInfo.backends.find(
+        backend => backend.site === locationObj.location);
+    if (!locMatch) {
+        repBackendResult.error = errors.InvalidArgument.customizeDescription(


Let's use InvalidLocationConstraint for this as well.

rahulreddy · 2018-04-16T22:48:08Z

lib/api/apiUtils/object/replicationBackendCompare.js

+        return repBackendResult;
+    }
+    if (locMatch.status === 'PENDING') {
+        repBackendResult.error = errors.InternalError.customizeDescription(


Please use errors.NoSuchKey here

rahulreddy · 2018-04-16T23:01:10Z

lib/api/apiUtils/object/replicationBackendCompare.js

+ * @param {string} locationObj.location - name of location constraint
+ * @param {string} locationObj.key - keyname of object in location constraint
+ * @param {string} locationObj.type - type of location constraint
+ * @param {object} replicationInfo - information about object replication


Please expand on the properties of this object.

rahulreddy · 2018-04-16T23:04:13Z

lib/api/apiUtils/object/locationConstraintCheck.js

+ * @param {object} headers - request headers
+ * @param {string} objectKey - key name of object
+ * @param {string} bucketName - name of bucket
+ * @return {object} - error if header location is not valid or object


Please expand on the properties of the return object and also include undefined as a return value. This seems to be the format generally used

@return {undefined|Object} @return {any} return.value The value of the attribute @return {boolean} return.modified If the value has been changed etc

rahulreddy · 2018-04-17T01:13:06Z

lib/api/objectGet.js

+        log.trace('invalid location constraint to get from', {
+            location: request.headers['x-amz-location-constraint'],
+            error: locCheckResult,
+        });


You can return the callback early avoid any further processing.

ironman-machine · 2018-04-17T05:28:50Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T05:43:28Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T20:04:03Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T21:46:17Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T23:38:28Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-17T23:38:28Z

CONFLICT (add/add): Merge conflict in tests/unit/multipleBackend/locationConstraintCheck.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/utils/gcpUtils.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/upload.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/putTagging.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/put.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/initiateMpu.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/head.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/getTagging.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/get.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/deleteTagging.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/deleteMpu.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/delete.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/copy.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/object/completeMpu.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/bucket/putVersioning.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/bucket/head.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/bucket/getVersioning.js
CONFLICT (add/add): Merge conflict in tests/functional/raw-node/test/GCP/bucket/get.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/utils.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/put/putGcp.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/put/put.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/objectTagging/taggingAwsVersioning-putget.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/objectTagging/taggingAwsVersioning-delete.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/objectTagging/objectTagging.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/objectPutCopyPart/objectPutCopyPartGcp.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/objectPutCopyPart/objectPutCopyPartAzure.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/objectCopy/objectCopyAwsVersioning.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/objectCopy/objectCopy.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/objectCopy/azureObjectCopy.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/mpuParts/putPartGcp.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/mpuComplete/mpuAwsVersioning.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/mpuComplete/completeMPUGcp.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/mpuComplete/azureCompleteMPU.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/mpuAbort/abortMPUGcp.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/listParts/listPartsGcp.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/listParts/azureListParts.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/initMPU/initMPUGcp.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/initMPU/initMPUAzure.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/get/getGcp.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/get/getAwsVersioning.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/get/get.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/delete/deleteGcp.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/delete/deleteAwsVersioning.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/delete/delete.js
CONFLICT (add/add): Merge conflict in tests/functional/aws-node-sdk/test/multipleBackend/acl/aclAwsVersioning.js
CONFLICT (add/add): Merge conflict in lib/api/objectGet.js
CONFLICT (add/add): Merge conflict in lib/api/apiUtils/object/locationConstraintCheck.js

jonathan-gramain · 2018-04-17T23:46:24Z

lib/api/apiUtils/object/replicationBackendCompare.js

+            'passed in location header');
+        return repBackendResult;
+    }
+    if (locMatch.status === 'PENDING') {


We may also want to handle FAILED status the same way, or alternatively, return the locator if status is COMPLETED, and fail otherwise and provide the replication status as an extra info in the returned message.

jonathan-gramain · 2018-04-17T23:58:11Z

lib/api/apiUtils/object/replicationBackendCompare.js

+ * @return {object} contains error if no replication backend matches or
+ * dataLocator object
+ */
+function replicationBackendCompare(locationObj, replicationInfo) {


What about renaming to getReplicationBackendDataLocator, since this is the main purpose of the function (and especially since the function is exported as a utility function potentially useful for others - thinking of transient source 😄).

ironman-machine · 2018-04-18T19:05:20Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-18T22:15:44Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-18T22:16:51Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-19T22:33:57Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-19T22:47:17Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-04-19T22:55:01Z

PR has been updated. Reviewers, please be cautious.

dora-korpar · 2018-04-19T22:55:13Z

@ironman-machine try

ironman-machine · 2018-04-19T22:55:17Z

Hello @dora-korpar

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/21340' with the following env. args:

{
    "DEFAULT_BRANCH": "master",
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "REPO_NAME": "S3",
    "SCALITY_S3_BRANCH": "ft/ZENKO-229-get-using-location"
}

bennettbuchanan

bennettbuchanan · 2018-04-19T23:47:24Z

lib/api/apiUtils/object/getReplicationBackendDataLocator.js

+    }
+    if (['PENDING', 'FAILED'].includes(locMatch.status)) {
+        repBackendResult.error = errors.NoSuchKey.customizeDescription(
+            'Object replication to specified backend is not complete');


I could imagine it being nice to include the status in the description, particularly if the status is failed. That way the user would know to retry the operation.

dora-korpar · 2018-04-20T17:00:18Z

@ironman-machine try

ironman-machine · 2018-04-20T17:00:19Z

Hello @dora-korpar

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/21351' with the following env. args:

{
    "DEFAULT_BRANCH": "master",
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "REPO_NAME": "S3",
    "SCALITY_S3_BRANCH": "ft/ZENKO-229-get-using-location"
}

dora-korpar · 2018-04-20T22:15:32Z

@ironman-machine try

ironman-machine · 2018-04-20T22:15:34Z

Hello @dora-korpar

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/21376' with the following env. args:

{
    "DEFAULT_BRANCH": "master",
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "REPO_NAME": "S3",
    "SCALITY_S3_BRANCH": "ft/ZENKO-229-get-using-location"
}

ironman-machine · 2018-04-23T17:33:51Z

PR has been updated. Reviewers, please be cautious.

dora-korpar · 2018-04-23T17:33:55Z

@ironman-machine try

ironman-machine · 2018-04-23T17:33:56Z

Hello @dora-korpar

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/21412' with the following env. args:

{
    "DEFAULT_BRANCH": "master",
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "REPO_NAME": "S3",
    "SCALITY_S3_BRANCH": "ft/ZENKO-229-get-using-location"
}

dora-korpar · 2018-04-23T20:33:06Z

@ironman-machine try

ironman-machine · 2018-04-23T20:33:10Z

Hello @dora-korpar

"try": Success: Try build successfully launched on 'http://ci.ironmann.io/gh/scality/Integration/21436' with the following env. args:

{
    "DEFAULT_BRANCH": "master",
    "SCALITY_INTEGRATION_BRANCH": "ultron/master",
    "REPO_NAME": "S3",
    "SCALITY_S3_BRANCH": "ft/ZENKO-229-get-using-location"
}

ironman-machine · 2018-04-23T21:52:02Z

☀️ 👍 circleCI test succeeded!

ironman-machine · 2018-04-23T22:21:56Z

PR has been updated. Reviewers, please be cautious.

Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)

ironman-machine · 2018-04-24T17:38:34Z

PR has been updated. Reviewers, please be cautious.

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from 926487e to 577c8fc Compare April 12, 2018 17:49

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from fade444 to 5c9801a Compare April 17, 2018 04:15

rahulreddy requested changes Apr 17, 2018

View reviewed changes

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from ba79b9b to d252705 Compare April 17, 2018 20:03

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from d252705 to 1f8fc75 Compare April 17, 2018 21:46

jonathan-gramain reviewed Apr 18, 2018

View reviewed changes

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from e5beef7 to a52ce02 Compare April 18, 2018 22:15

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from a52ce02 to b102bfa Compare April 18, 2018 22:16

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from b102bfa to 6141c35 Compare April 19, 2018 22:33

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from 6141c35 to 5cf6e08 Compare April 19, 2018 22:47

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from 5cf6e08 to 4ad2032 Compare April 19, 2018 22:54

dora-korpar changed the title ~~Ft/ZENKO-229 get using location~~ ft/ZENKO-229 get using location Apr 19, 2018

bennettbuchanan previously approved these changes Apr 19, 2018

View reviewed changes

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from a727ebc to 6c6a51e Compare April 23, 2018 17:33

jonathan-gramain previously approved these changes Apr 23, 2018

View reviewed changes

dora-korpar dismissed jonathan-gramain’s stale review via 719795e April 23, 2018 22:21

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from 6c6a51e to 719795e Compare April 23, 2018 22:21

dora-korpar changed the base branch from master to z/1.0 April 23, 2018 22:22

jonathan-gramain previously approved these changes Apr 23, 2018

View reviewed changes

alexanderchan-scality previously approved these changes Apr 23, 2018

View reviewed changes

rahulreddy added the prio-high label Apr 24, 2018

rahulreddy previously approved these changes Apr 24, 2018

View reviewed changes

ft: ZENKO-229 get obj based on location

d6e8201

dora-korpar force-pushed the ft/ZENKO-229-get-using-location branch from 719795e to d6e8201 Compare April 24, 2018 17:38

rahulreddy approved these changes Apr 24, 2018

View reviewed changes

alexanderchan-scality approved these changes Apr 24, 2018

View reviewed changes

nicolas2bert merged commit d52762c into z/1.0 Apr 25, 2018

rahulreddy deleted the ft/ZENKO-229-get-using-location branch April 26, 2018 05:48

ft/ZENKO-229 get using location #1204

ft/ZENKO-229 get using location #1204

Uh oh!

Conversation

dora-korpar commented Apr 12, 2018

Uh oh!

ironman-machine commented Apr 12, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

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!

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!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

ironman-machine commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironman-machine commented Apr 18, 2018

Uh oh!

ironman-machine commented Apr 18, 2018

Uh oh!

ironman-machine commented Apr 18, 2018

Uh oh!

ironman-machine commented Apr 19, 2018

Uh oh!

ironman-machine commented Apr 19, 2018

Uh oh!

ironman-machine commented Apr 19, 2018

Uh oh!

dora-korpar commented Apr 19, 2018

Uh oh!

ironman-machine commented Apr 19, 2018

Uh oh!

bennettbuchanan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dora-korpar commented Apr 20, 2018

Uh oh!

ironman-machine commented Apr 20, 2018

Uh oh!

dora-korpar commented Apr 20, 2018

Uh oh!

ironman-machine commented Apr 20, 2018

Uh oh!

ironman-machine commented Apr 23, 2018

Uh oh!

dora-korpar commented Apr 23, 2018

Uh oh!

ironman-machine commented Apr 23, 2018