Compare commits

...

2 Commits

Author SHA1 Message Date
Jonathan Gramain da038558a8 wip 2022-09-19 16:20:57 -07:00
Jonathan Gramain bbca8ff03f Revert "ARSN-252 - listing bug in DelimisterMaster"
This reverts commit f62c3d22ed.
2022-09-16 17:05:24 -07:00
8 changed files with 118 additions and 257 deletions

View File

@ -2,9 +2,10 @@
const Extension = require('./Extension').default;
const { inc, listingParamsMasterKeysV0ToV1,
FILTER_END, FILTER_ACCEPT, FILTER_SKIP } = require('./tools');
FILTER_END, FILTER_ACCEPT, FILTER_SKIP, SKIP_NONE } = require('./tools');
const VSConst = require('../../versioning/constants').VersioningConstants;
const { DbPrefixes, BucketVersioningKeyFormat } = VSConst;
const VID_SEP = VSConst.VersionId.Separator;
/**
* Find the common prefix in the path
@ -64,6 +65,7 @@ class Delimiter extends Extension {
this.maxKeys = parameters.maxKeys || 1000;
this.startAfter = parameters.startAfter;
this.continuationToken = parameters.continuationToken;
this.prefixToSkip = SKIP_NONE;
this.alphabeticalOrder =
typeof parameters.alphabeticalOrder !== 'undefined' ?
parameters.alphabeticalOrder : true;
@ -189,6 +191,8 @@ class Delimiter extends Extension {
&& key <= this[this.nextContinueMarker])) {
return FILTER_SKIP;
}
// reset `prefixToSkip`, it may be updated if `addCommonPrefix()` is called
this.prefixToSkip = SKIP_NONE;
if (this.delimiter) {
const baseIndex = this.prefix ? this.prefix.length : 0;
const delimiterIndex = key.indexOf(this.delimiter, baseIndex);
@ -208,13 +212,14 @@ class Delimiter extends Extension {
*/
addCommonPrefix(key, index) {
const commonPrefix = getCommonPrefix(key, this.delimiter, index);
this[this.nextContinueMarker] = key;
this.prefixToSkip = commonPrefix;
if (this.CommonPrefixes.indexOf(commonPrefix) === -1
&& this[this.nextContinueMarker] !== commonPrefix) {
if (this._reachedMaxKeys()) {
return FILTER_END;
}
this.CommonPrefixes.push(commonPrefix);
this[this.nextContinueMarker] = commonPrefix;
++this.keys;
return FILTER_ACCEPT;
}
@ -229,7 +234,7 @@ class Delimiter extends Extension {
* that it's enough and should move on
*/
skippingV0() {
return this[this.nextContinueMarker];
return this.prefixToSkip;
}
/**
@ -240,7 +245,7 @@ class Delimiter extends Extension {
* that it's enough and should move on
*/
skippingV1() {
return DbPrefixes.Master + this[this.nextContinueMarker];
return this.prefixToSkip ? DbPrefixes.Master + this.prefixToSkip : SKIP_NONE;
}
/**
@ -271,4 +276,4 @@ class Delimiter extends Extension {
}
}
module.exports = { Delimiter, getCommonPrefix };
module.exports = { Delimiter };

View File

@ -41,7 +41,6 @@ class DelimiterMaster extends Delimiter {
},
[BucketVersioningKeyFormat.v1]: {
filter: this.filterV1,
skipping: this.skippingV1,
},
}[this.vFormat]);
}
@ -62,10 +61,6 @@ class DelimiterMaster extends Delimiter {
let key = obj.key;
const value = obj.value;
if (key === this.prefix) {
return this.addContents(key, value);
}
if (key.startsWith(DbPrefixes.Replay)) {
this.inReplayPrefix = true;
return FILTER_SKIP;
@ -94,17 +89,17 @@ class DelimiterMaster extends Delimiter {
* in the constructor and comparing to NextMarker is the only
* way to know we should not accept this version. This test is
* not redundant with the one at the beginning of this function,
* we are comparing here the key without the version suffix,
* - key startsWith the previous NextMarker happens because we set
* NextMarker to the common prefix instead of the whole key
* value. (TODO: remove this test once ZENKO-1048 is fixed)
* we are comparing here the key without the version suffix.
* */
if (key === this.prvKey || key === this[this.nextContinueMarker] ||
(this.delimiter && key.startsWith(this[this.nextContinueMarker]))) {
if (key === this.prvKey || key === this[this.nextContinueMarker]) {
/* master version already filtered */
return FILTER_SKIP;
}
}
// If addCommonPrefix() gets called in this function, it will
// set `prefixToSkip` to the current prefix, otherwise in the
// general case we may skip all further versions of that key.
this.prefixToSkip = key + VID_SEP;
if (Version.isPHD(value)) {
/* master version is a PHD version, we want to wait for the next
* one:
@ -165,34 +160,11 @@ class DelimiterMaster extends Delimiter {
return super.filter(obj);
}
skippingBase() {
if (this[this.nextContinueMarker]) {
// next marker or next continuation token:
// - foo/ : skipping foo/
// - foo : skipping foo.
const index = this[this.nextContinueMarker].
lastIndexOf(this.delimiter);
if (index === this[this.nextContinueMarker].length - 1) {
return this[this.nextContinueMarker];
}
return this[this.nextContinueMarker] + VID_SEP;
}
return SKIP_NONE;
}
skippingV0() {
if (this.inReplayPrefix) {
return DbPrefixes.Replay;
}
return this.skippingBase();
}
skippingV1() {
const skipTo = this.skippingBase();
if (skipTo === SKIP_NONE) {
return SKIP_NONE;
}
return DbPrefixes.Master + skipTo;
return super.skippingV0();
}
}

View File

@ -238,13 +238,7 @@ class DelimiterVersions extends Delimiter {
if (this.inReplayPrefix) {
return DbPrefixes.Replay;
}
if (this.NextMarker) {
const index = this.NextMarker.lastIndexOf(this.delimiter);
if (index === this.NextMarker.length - 1) {
return this.NextMarker;
}
}
return SKIP_NONE;
return this.prefixToSkip;
}
skippingV1() {

View File

@ -62,7 +62,6 @@
"@types/jest": "^27.4.1",
"@types/node": "^17.0.21",
"@types/xml2js": "^0.4.11",
"chance": "^1.1.8",
"eslint": "^8.12.0",
"eslint-config-airbnb": "6.2.0",
"eslint-config-scality": "scality/Guidelines#7.10.2",

View File

@ -1,9 +1,7 @@
'use strict'; // eslint-disable-line strict
const assert = require('assert');
const chance = require('chance').Chance(); // eslint-disable-line
const { getCommonPrefix } = require('../../../../lib/algos/list/delimiter');
const DelimiterMaster =
require('../../../../lib/algos/list/delimiterMaster').DelimiterMaster;
const {
@ -18,6 +16,8 @@ const Version = require('../../../../lib/versioning/Version').Version;
const { generateVersionId } = require('../../../../lib/versioning/VersionID');
const { DbPrefixes } = VSConst;
const zpad = require('../../helpers').zpad;
const VID_SEP = VSConst.VersionId.Separator;
const EmptyResult = {
CommonPrefixes: [],
@ -46,111 +46,6 @@ function getListingKey(key, vFormat) {
return assert.fail(`bad vFormat ${vFormat}`);
}
const MAX_STREAK_LENGTH = 100;
function createStreakState(prefix, vFormat) {
return {
vFormat,
prefix,
prefixes: new Set(),
params: {},
streakLength: 0,
gteparams: null,
};
}
/**
* Simplified version of logic found in Metadata's RepdServer. When MAX_STREAK_LENGTH (typically 100)
* is reached, we attempt to skip to the next listing range as a performance optimization.
* @param {Integer} filteringResult - 0, 1, -1 indicating if we can skip to the next character range.
* @param {String} skippingRange - lower bound that the listing can begin from.
* @returns {undefined}
*/
/* eslint-disable no-param-reassign */
function handleStreak(filteringResult, skippingRange, state) {
if (filteringResult < 0) {
// readStream would be destroyed. In this mock, we just continue.
} else if (filteringResult === 0 && skippingRange) {
// check if MAX_STREAK_LENGTH consecutive keys have been
// skipped
if (++state.streakLength === MAX_STREAK_LENGTH) {
if (Array.isArray(skippingRange)) {
// With synchronized listing, the listing
// algo backend skipping() function must
// return as many skip keys as listing
// param sets.
for (let i = 0; i < skippingRange.length; ++i) {
state.params[i].gte = inc(skippingRange[i]);
}
} else {
state.params.gte = inc(skippingRange);
}
if (state.gteparams && state.gteparams === state.params.gte) {
state.streakLength = 1;
} else {
// stop listing this key range
state.gteparams = state.params.gte;
}
}
} else {
state.streakLength = 1;
}
}/* eslint-enable */
/**
* Generate a random number of versioned keys.
* @param {String} masterKey - base key used to derive version keys
* @param {Integer} numKeys - how many versioned keys to generate
* @param {Object} state - test case state
* @yields {Object} - { key, isDeleteMarker }
* @returns {undefined}
*/
function *generateVersionedKeys(masterKey, numKeys, state) {
for (let i = 0; i < numKeys; i++) {
yield {
key: `${masterKey}${VID_SEP}${zpad(i)}`,
isDeleteMarker: state.vFormat === 'v0' && chance.bool(),
canSkip: true,
};
}
}
/**
* Generate raw listing keys in alphabetical order.
* @param {Integer} count - how many keys to generate.
* @param {Object} state - state for test run.
* @yields {Object} - { key, isDeleteMarker }
*/
function *generateKeys(count, state) {
let idx = 0;
let masterKey = '_Common-Prefix/';
yield { key: masterKey, isDeleteMarker: false, canSkip: false };
idx++;
while (idx < count) {
masterKey = `_Common-Prefix/${zpad(idx)}`;
const masterKeyWithSubprefix = `_Common-Prefix/${zpad(idx)}/${zpad(idx)}`;
const hasSubprefix = chance.bool();
const isDeleteMarker = state.vFormat === 'v0' && chance.bool();
const baseKey = hasSubprefix ? masterKeyWithSubprefix : masterKey;
let canSkip = isDeleteMarker;
if (hasSubprefix || state.prefix[state.prefix.length - 1] !== '/') {
canSkip = canSkip || state.handleSubprefixKey(baseKey);
}
yield { key: baseKey, isDeleteMarker, canSkip };
idx++;
const versioned = chance.integer({ min: 0, max: 200 });
const allowedVersionedKeys = Math.min(count - idx, versioned);
yield* generateVersionedKeys(baseKey, allowedVersionedKeys, state);
idx += allowedVersionedKeys;
}
}
['v0', 'v1'].forEach(vFormat => {
describe(`Delimiter All masters listing algorithm vFormat=${vFormat}`, () => {
it('should return SKIP_NONE for DelimiterMaster when both NextMarker ' +
@ -164,71 +59,7 @@ function *generateKeys(count, state) {
assert.strictEqual(delimiter.skipping(), SKIP_NONE);
});
it('should list all subprefixes when handleStreak is applied as in RepdServer', () => {
['_Common-Prefix', '_Common-Prefix/'].forEach(prefix => {
const state = createStreakState(prefix, vFormat);
state.handleSubprefixKey = baseKey => {
const isLastDelim = state.prefix[state.prefix.length - 1] === '/';
const lastIdx = isLastDelim ? baseKey.lastIndexOf('/') : state.prefix.length;
const prefix = isLastDelim ? getCommonPrefix(baseKey, '/', lastIdx) : baseKey.slice(0, lastIdx);
if (state.prefixes.has(prefix) || (!isLastDelim && baseKey.length > lastIdx)) {
return true;
}
state.prefixes.add(prefix);
return false;
};
const maxKeys = 1000000;
const delimiter = new DelimiterMaster({
maxKeys,
prefix,
delimiter: '/',
startAfter: '',
continuationToken: '',
v2: true,
fetchOwner: false }, fakeLogger, vFormat);
[...generateKeys(maxKeys, state)].forEach(ob => {
const { key, isDeleteMarker, canSkip } = ob;
// simulate not doing a raw listing when key < params.gte
// this represents a key not reaching the read stream from leveldb
if (state.params.gte && key < state.params.gte) {
return;
}
if (!key.includes(VID_SEP)) { // master key
const version = new Version({ isDeleteMarker });
const obj = {
key: getListingKey(key, vFormat),
value: version.toString(),
};
const res = delimiter.filter(obj);
const skippingRange = delimiter.skipping();
handleStreak(res, skippingRange, state);
const expected = canSkip ? FILTER_SKIP : FILTER_ACCEPT;
assert.strictEqual(res, expected);
} else { // versioned key
if (vFormat === 'v0') {
const vid = key.split(VID_SEP).slice(-1)[0];
const version = new Version({ versionId: vid, isDeleteMarker });
const obj2 = {
key: getListingKey(key, vFormat),
value: version.toString(),
};
const res = delimiter.filter(obj2);
const skippingRange = delimiter.skipping();
handleStreak(res, skippingRange, state);
assert.strictEqual(res, FILTER_SKIP);
}
}
});
});
});
it('should return <key><VersionIdSeparator> for DelimiterMaster when ' +
it('should return good skipping value for DelimiterMaster when ' +
'NextMarker is set and there is a delimiter', () => {
const key = 'key';
const delimiter = new DelimiterMaster({ delimiter: '/', marker: key },
@ -239,13 +70,16 @@ function *generateKeys(count, state) {
delimiter.filter({ key: listingKey, value: '' });
assert.strictEqual(delimiter.NextMarker, key);
/* With a delimiter skipping should return previous key + VID_SEP
* (except when a delimiter is set and the NextMarker ends with the
* delimiter) . */
assert.strictEqual(delimiter.skipping(), listingKey + VID_SEP);
if (vFormat === 'v0') {
// With a delimiter skipping should return previous key + VID_SEP in v0
assert.strictEqual(delimiter.skipping(), listingKey + VID_SEP);
} else {
// in v1 there are no versions to skip
assert.strictEqual(delimiter.skipping(), SKIP_NONE);
}
});
it('should return <key><VersionIdSeparator> for DelimiterMaster when ' +
it('should return good skipping value for DelimiterMaster when ' +
'NextContinuationToken is set and there is a delimiter', () => {
const key = 'key';
const delimiter = new DelimiterMaster(
@ -257,11 +91,17 @@ function *generateKeys(count, state) {
delimiter.filter({ key: listingKey, value: '' });
assert.strictEqual(delimiter.NextContinuationToken, key);
assert.strictEqual(delimiter.skipping(), listingKey + VID_SEP);
if (vFormat === 'v0') {
// With a delimiter skipping should return previous key + VID_SEP in v0
assert.strictEqual(delimiter.skipping(), listingKey + VID_SEP);
} else {
// in v1 there are no versions to skip
assert.strictEqual(delimiter.skipping(), SKIP_NONE);
}
});
it('should return NextMarker for DelimiterMaster when NextMarker is set' +
', there is a delimiter and the key ends with the delimiter', () => {
it('should return SKIP_NONE for DelimiterMaster when NextMarker is set' +
', there is a delimiter and the filtered key ends with the delimiter', () => {
const delimiterChar = '/';
const keyWithEndingDelimiter = `key${delimiterChar}`;
const delimiter = new DelimiterMaster({
@ -269,13 +109,10 @@ function *generateKeys(count, state) {
marker: keyWithEndingDelimiter,
}, fakeLogger, vFormat);
/* When a delimiter is set and the NextMarker ends with the
* delimiter it should return the next marker value. */
/* When a NextMarker is set to the key, we should get
* SKIP_NONE because no key has been listed yet. */
assert.strictEqual(delimiter.NextMarker, keyWithEndingDelimiter);
const skipKey = vFormat === 'v1' ?
`${DbPrefixes.Master}${keyWithEndingDelimiter}` :
keyWithEndingDelimiter;
assert.strictEqual(delimiter.skipping(), skipKey);
assert.strictEqual(delimiter.skipping(), SKIP_NONE);
});
it('should skip entries not starting with prefix', () => {
@ -609,22 +446,6 @@ function *generateKeys(count, state) {
});
});
it('should skip a versioned entry when there is a delimiter and the key ' +
'starts with the NextMarker value', () => {
const delimiterChar = '/';
const commonPrefix = `commonPrefix${delimiterChar}`;
const key = `${commonPrefix}key${VID_SEP}version`;
const value = 'value';
const delimiter = new DelimiterMaster({ delimiter: delimiterChar },
fakeLogger, vFormat);
/* TODO: should be set to a whole key instead of just a common prefix
* once ZENKO-1048 is fixed. */
delimiter.NextMarker = commonPrefix;
assert.strictEqual(delimiter.filter({ key, value }), FILTER_SKIP);
});
it('should return good skipping value for DelimiterMaster on replay keys', () => {
const delimiter = new DelimiterMaster(
{ delimiter: '/', v2: true },
@ -657,6 +478,81 @@ function *generateKeys(count, state) {
// ...it should return to skipping by prefix as usual
assert.strictEqual(delimiter.skipping(), `${inc(DbPrefixes.Replay)}foo/`);
});
it('should not skip over whole prefix when a key equals the prefix', () => {
const FILTER_MAP = {
'0': 'FILTER_SKIP',
'1': 'FILTER_ACCEPT',
'-1': 'FILTER_END',
};
const delimiter = new DelimiterMaster({
prefix: 'prefix/',
delimiter: '/',
}, fakeLogger, vFormat);
for (const testEntry of [
{
key: 'prefix/',
expectedRes: FILTER_ACCEPT,
expectedSkipping: `prefix/${VID_SEP}`,
},
{
key: `prefix/${VID_SEP}v1`,
value: '{}',
expectedRes: FILTER_SKIP, // versions get skipped after master
expectedSkipping: `prefix/${VID_SEP}`,
},
{
key: 'prefix/deleted',
isDeleteMarker: true,
expectedRes: FILTER_SKIP, // delete markers get skipped
expectedSkipping: `prefix/deleted${VID_SEP}`,
},
{
key: `prefix/deleted${VID_SEP}v1`,
isDeleteMarker: true,
expectedRes: FILTER_SKIP,
expectedSkipping: `prefix/deleted${VID_SEP}`,
},
{
key: `prefix/deleted${VID_SEP}v2`,
expectedRes: FILTER_SKIP,
expectedSkipping: `prefix/deleted${VID_SEP}`,
},
{
key: 'prefix/subprefix1/key-1',
expectedRes: FILTER_ACCEPT,
expectedSkipping: 'prefix/subprefix1/',
},
{
key: `prefix/subprefix1/key-1${VID_SEP}v1`,
expectedRes: FILTER_SKIP,
expectedSkipping: 'prefix/subprefix1/',
},
{
key: 'prefix/subprefix1/key-2',
expectedRes: FILTER_SKIP,
expectedSkipping: 'prefix/subprefix1/',
},
{
key: `prefix/subprefix1/key-2${VID_SEP}v1`,
expectedRes: FILTER_SKIP,
expectedSkipping: 'prefix/subprefix1/',
},
]) {
const entry = {
key: testEntry.key,
};
if (testEntry.isDeleteMarker) {
entry.value = '{"isDeleteMarker":true}';
} else {
entry.value = '{}';
}
const res = delimiter.filter(entry);
const skipping = delimiter.skipping();
assert.strictEqual(res, testEntry.expectedRes);
assert.strictEqual(skipping, testEntry.expectedSkipping);
}
});
}
});
});

View File

@ -406,7 +406,7 @@ const tests = [
}],
}, {
Versions: [],
CommonPrefixes: ['notes/summer/'],
CommonPrefixes: ['notes/spring/'],
Delimiter: '/',
IsTruncated: true,
NextKeyMarker: 'notes/summer/',

View File

@ -166,7 +166,7 @@ describe('network.Server: ', () => {
if (err) {
return ws.onStop(() => {
clearTimeout(requestTimeout);
if (err.code === 'EPROTO' || err.code === 'ECONNRESET') {
if (err.code === 'EPROTO') {
return done();
}
return done(err);

View File

@ -2144,11 +2144,6 @@ chalk@^4.0.0:
ansi-styles "^4.1.0"
supports-color "^7.1.0"
chance@^1.1.8:
version "1.1.8"
resolved "https://registry.yarnpkg.com/chance/-/chance-1.1.8.tgz#5d6c2b78c9170bf6eb9df7acdda04363085be909"
integrity sha512-v7fi5Hj2VbR6dJEGRWLmJBA83LJMS47pkAbmROFxHWd9qmE1esHRZW8Clf1Fhzr3rjxnNZVCjOEv/ivFxeIMtg==
char-regex@^1.0.2:
version "1.0.2"
resolved "https://registry.yarnpkg.com/char-regex/-/char-regex-1.0.2.tgz#d744358226217f981ed58f479b1d6bcc29545dcf"