8000 fix invalid handling of `_lastValue` in case of multiple coordinators… · HighAvailabilityLab/arangodb@a19f90c · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit a19f90c

Browse files
jsteemannjsteemann
authored
fix invalid handling of _lastValue in case of multiple coordinators (arangodb#7734)
1 parent 7eed661 commit a19f90c

File tree

4 files changed

+193
-88
lines changed

4 files changed

+193
-88
lines changed

‎arangod/VocBase/KeyGenerator.cpp

Lines changed: 151 additions & 69 deletions
F438
Original file line numberDiff line numberDiff line change
@@ -132,33 +132,23 @@ class TraditionalKeyGenerator : public KeyGenerator {
132132
public:
133133
/// @brief create the generator
134134
explicit TraditionalKeyGenerator(bool allowUserKeys)
135-
: KeyGenerator(allowUserKeys),
136-
_lastValue(0) {}
135+
: KeyGenerator(allowUserKeys) {}
137136

138137
bool hasDynamicState() const override { return true; }
139-
138+
140139
/// @brief generate a key
141140
std::string generate() override final {
142141
uint64_t tick = generateValue();
143142

144-
{
145-
MUTEX_LOCKER(mutexLocker, _lock);
146-
147-
if (tick == UINT64_MAX || _lastValue >= UINT64_MAX - 1ULL) {
148-
// oops, out of keys!
149-
return std::string();
150-
}
151-
152-
if (tick <= _lastValue) {
153-
tick = ++_lastValue;
154-
} else {
155-
_lastValue = tick;
156-
}
143+
if (ADB_UNLIKELY(tick == 0)) {
144+
// unlikely case we have run out of keys
145+
// returning an empty string will trigger an error on the call site
146+
return std::string();
157147
}
158148

159149
return arangodb::basics::StringUtils::itoa(tick);
160150
}
161-
151+
162152
/// @brief validate a key
163153
int validate(char const* p, size_t length, bool isRestore) override {
164154
int res = KeyGenerator::validate(p, length, isRestore);
@@ -180,12 +170,7 @@ class TraditionalKeyGenerator : public KeyGenerator {
180170
uint64_t value = NumberUtils::atoi_zero<uint64_t>(p, p + length);
181171

182172
if (value > 0) {
183-
MUTEX_LOCKER(mutexLocker, _lock);
184-
185-
if (value > _lastValue) {
186-
// and update our last value
187-
_lastValue = value;
188-
}
173+
track(value);
189174
}
190175
}
191176
}
@@ -194,76 +179,123 @@ class TraditionalKeyGenerator : public KeyGenerator {
194179
void toVelocyPack(arangodb::velocypack::Builder& builder) const override {
195180
KeyGenerator::toVelocyPack(builder);
196181
builder.add("type", VPackValue("traditional"));
197-
198-
MUTEX_LOCKER(mutexLocker, _lock);
199-
builder.add(StaticStrings::LastValue, VPackValue(_lastValue));
200182
}
201183

202184
protected:
185+
/// @brief generate a new key value (internal)
203186
virtual uint64_t generateValue() = 0;
204187

205-
private:
206-
mutable arangodb::Mutex _lock;
207-
208-
uint64_t _lastValue;
188+
/// @brief track a value (internal)
189+
virtual void track(uint64_t value) = 0;
209190
};
210191

211192
/// @brief traditional key generator for a single server
212193
class TraditionalKeyGeneratorSingle final : public TraditionalKeyGenerator {
213194
public:
214195
/// @brief create the generator
215196
explicit TraditionalKeyGeneratorSingle(bool allowUserKeys)
216-
: TraditionalKeyGenerator(allowUserKeys) {}
197+
: TraditionalKeyGenerator(allowUserKeys), _lastValue(0) {
198+
TRI_ASSERT(!ServerState::instance()->isCoordinator());
199+
}
200+
201+
/// @brief build a VelocyPack representation of the generator in the builder
202+
void toVelocyPack(arangodb::velocypack::Builder& builder) const override {
203+
TraditionalKeyGenerator::toVelocyPack(builder);
204+
205+
// add our specific stuff
206+
MUTEX_LOCKER(mutexLocker, _lock);
207+
builder.add(StaticStrings::LastValue, VPackValue(_lastValue));
208+
}
217209

218210
private:
219-
/// @brief generate a key
211+
/// @brief generate a key value (internal)
220212
uint64_t generateValue() override {
221-
return TRI_NewTickServer();
213+
uint64_t tick = TRI_NewTickServer();
214+
215+
if (ADB_UNLIKELY(tick == UINT64_MAX)) {
216+
// out of keys
217+
return 0;
218+
}
219+
220+
// keep track of last assigned value, and make sure the value
221+
// we hand out is always higher than it
222+
{
223+
MUTEX_LOCKER(mutexLocker, _lock);
224+
225+
if (ADB_UNLIKELY(_lastValue >= UINT64_MAX - 1ULL)) {
226+
// oops, out of keys!
227+
return 0;
228+
}
229+
230+
if (tick <= _lastValue) {
231+
tick = ++_lastValue;
232+
} else {
233+
_lastValue = tick;
234+
}
235+
}
236+
237+
return tick;
238+
}
239+
240+
/// @brief track a key value (internal)
241+
void track(uint64_t value) override {
242+
MUTEX_LOCKER(mutexLocker, _lock);
243+
244+
if (value > _lastValue) {
245+
// and update our last value
246+
_lastValue = value;
247+
}
222248
}
249+
250+
private:
251+
mutable arangodb::Mutex _lock;
252+
253+
uint64_t _lastValue;
223254
};
224255

225256
/// @brief traditional key generator for a coordinator
257+
/// please note that coordinator-based key generators are frequently
258+
/// created and discarded, so ctor & dtor need to be very efficient.
259+
/// additionally, do not put any state into this object, as for the
260+
/// same logical collection the ClusterInfo may create many different
261+
/// temporary LogicalCollection objects one after the other, which
262+
/// will also discard the collection's particular KeyGenerator object!
226263
class TraditionalKeyGeneratorCluster final : public TraditionalKeyGenerator {
227264
public:
228265
/// @brief create the generator
229266
explicit TraditionalKeyGeneratorCluster(bool allowUserKeys)
230-
: TraditionalKeyGenerator(allowUserKeys) {}
231-
267+
: TraditionalKeyGenerator(allowUserKeys) {
268+
TRI_ASSERT(ServerState::instance()->isCoordinator());
269+
}
270+
232271
private:
233-
/// @brief generate a key
272+
/// @brief generate a key value (internal)
234273
uint64_t generateValue() override {
235274
ClusterInfo* ci = ClusterInfo::instance();
236275
return ci->uniqid();
237276
}
277+
278+
/// @brief track a key value (internal)
279+
void track(uint64_t /* value */) override {}
238280
};
239281

240282
/// @brief base class for padded key generators
241283
class PaddedKeyGenerator : public KeyGenerator {
242284
public:
243285
/// @brief create the generator
244286
explicit PaddedKeyGenerator(bool allowUserKeys)
245-
: KeyGenerator(allowUserKeys),
246-
_lastValue(0) {}
287+
: KeyGenerator(allowUserKeys) {}
247288

248289
bool hasDynamicState() const override { return true; }
249290

250291
/// @brief generate a key
251292
std::string generate() override {
252293
uint64_t tick = generateValue();
253-
254-
{
255-
MUTEX_LOCKER(mutexLocker, _lock);
256-
257-
if (tick == UINT64_MAX || _lastValue >= UINT64_MAX - 1ULL) {
258-
// oops, out of keys!
259-
return std::string();
260-
}
261294

262-
if (tick <= _lastValue) {
263-
tick = ++_lastValue;
264-
} else {
265-
_lastValue = tick;
266-
}
295+
if (ADB_UNLIKELY(tick == 0)) {
296+
// unlikely case we have run out of keys
297+
// returning an empty string will trigger an error on the call site
298+
return std::string();
267299
}
268300

269301
return encode(tick);
@@ -287,26 +319,22 @@ class PaddedKeyGenerator : public KeyGenerator {
287319
// check the numeric key part
288320
uint64_t value = decode(p, length);
289321
if (value > 0) {
290-
MUTEX_LOCKER(mutexLocker, _lock);
291-
292-
if (value > _lastValue) {
293-
// and update our last value
294-
_lastValue = value;
295-
}
322+
track(value);
296323
}
297324
}
298325

299326
/// @brief build a VelocyPack representation of the generator in the builder
300327
void toVelocyPack(arangodb::velocypack::Builder& builder) const override {
301328
KeyGenerator::toVelocyPack(builder);
302329
builder.add("type", VPackValue("padded"));
303-
304-
MUTEX_LOCKER(mutexLocker, _lock);
305-
builder.add(StaticStrings::LastValue, VPackValue(_lastValue));
306330
}
307331

308332
protected:
333+
/// @brief generate a key value (internal)
309334
virtual uint64_t generateValue() = 0;
335+
336+
/// @brief track a value (internal)
337+
virtual void track(uint64_t value) = 0;
310338

311339
private:
312340
uint64_t decode(char const* p, size_t length) {
@@ -360,40 +388,94 @@ class PaddedKeyGenerator : public KeyGenerator {
360388

361389
return std::string(&buffer[0], sizeof(uint64_t) * 2);
362390
}
363-
364-
private:
365-
mutable arangodb::Mutex _lock;
366-
367-
uint64_t _lastValue;
368391
};
369392

370393
/// @brief padded key generator for a single server
371394
class PaddedKeyGeneratorSingle final : public PaddedKeyGenerator {
372395
public:
373396
/// @brief create the generator
374397
explicit PaddedKeyGeneratorSingle(bool allowUserKeys)
375-
: PaddedKeyGenerator(allowUserKeys) {}
398+
: PaddedKeyGenerator(allowUserKeys), _lastValue(0) {
399+
TRI_ASSERT(!ServerState::instance()->isCoordinator());
400+
}
401+
402+
/// @brief build a VelocyPack representation of the generator in the builder
403+
void toVelocyPack(arangodb::velocypack::Builder& builder) const override {
404+
PaddedKeyGenerator::toVelocyPack(builder);
405+
406+
// add our own specific values
407+
MUTEX_LOCKER(mutexLocker, _lock);
408+
builder.add(StaticStrings::LastValue, VPackValue(_lastValue));
409+
}
376410

377411
private:
378412
/// @brief generate a key
379413
uint64_t generateValue() override {
380-
return TRI_NewTickServer();
414+
uint64_t tick = TRI_NewTickServer();
415+
416+
if (ADB_UNLIKELY(tick == UINT64_MAX)) {
417+
// oops, out of keys!
418+
return 0;
419+
}
420+
421+
{
422+
MUTEX_LOCKER(mutexLocker, _lock);
423+
424+
if (ADB_UNLIKELY(_lastValue >= UINT64_MAX - 1ULL)) {
425+
// oops, out of keys!
426+
return 0;
427+
}
428+
429+
if (tick <= _lastValue) {
430+
tick = ++_lastValue;
431+
} else {
432+
_lastValue = tick;
433+
}
434+
}
435+
436+
return tick;
437+
}
438+
439+
/// @brief generate a key value (internal)
440+
void track(uint64_t value) override {
441+
MUTEX_LOCKER(mutexLocker, _lock);
442+
443+
if (value > _lastValue) {
444+
// and update our last value
445+
_lastValue = value;
446+
}
381447
}
448+
449+
private:
450+
mutable arangodb::Mutex _lock;
451+
452+
uint64_t _lastValue;
382453
};
383454

384455
/// @brief padded key generator for a coordinator
456+
/// please note that coordinator-based key generators are frequently
457+
/// created and discarded, so ctor & dtor need to be very efficient.
458+
/// additionally, do not put any state into this object, as for the
459+
/// same logical collection the ClusterInfo may create many different
460+
/// temporary LogicalCollection objects one after the other, which
461+
/// will also discard the collection's particular KeyGenerator object!
385462
class PaddedKeyGeneratorCluster final : public PaddedKeyGenerator {
386463
public:
387464
/// @brief create the generator
388465
explicit PaddedKeyGeneratorCluster(bool allowUserKeys)
389-
: PaddedKeyGenerator(allowUserKeys) {}
466+
: PaddedKeyGenerator(allowUserKeys) {
467+
TRI_ASSERT(ServerState::instance()->isCoordinator());
468+
}
390469

391470
private:
392-
/// @brief generate a key
471+
/// @brief generate a key value (internal)
393472
uint64_t generateValue() override {
394473
ClusterInfo* ci = ClusterInfo::instance();
395474
return ci->uniqid();
396475
}
476+
477+
/// @brief generate a key value (internal)
478+
void track(uint64_t /* value */) override {}
397479
};
398480

399481
/// @brief auto-increment key generator - not usable in cluster

‎arangod/VocBase/KeyGenerator.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,14 @@ class Builder;
3535
class Slice;
3636
}
3737

38+
/// generic key generator interface
39+
///
40+
/// please note that coordinator-based key generators are frequently
41+
/// created and discarded, so ctor & dtor need to be very efficient.
42+
/// additionally, do not put any state into this object, as for the
43+
/// same logical collection the ClusterInfo may create many different
44+
/// temporary LogicalCollection objects one after the other, which
45+
/// will also discard the collection's particular KeyGenerator object!
3846
class KeyGenerator {
3947
KeyGenerator(KeyGenerator const&) = delete;
4048
KeyGenerator& operator=(KeyGenerator const&) = delete;
@@ -55,6 +63,8 @@ class KeyGenerator {
5563
virtual bool hasDynamicState() const { return true; }
5664

5765
/// @brief generate a key
66+
/// if the returned string is empty, it means no proper key was
67+
/// generated, and the caller must handle the situation
5868
virtual std::string generate() = 0;
5969

6070
/// @brief validate a key

‎arangod/VocBase/LogicalCollection.h

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,14 @@ class ChecksumResult : public Result {
7575
velocypack::Builder _builder;
7676
};
7777

78+
/// please note that coordinator-based logical collections are frequently
79+
/// created and discarded, so ctor & dtor need to be as efficient as possible.
80+
/// additionally, do not put any volatile state into this object in the coordinator,
81+
/// as the ClusterInfo may create many different temporary physical LogicalCollection
82+
/// objects (one after the other) even for the same "logical" LogicalCollection.
83+
/// this which will also discard the collection's volatile state each time!
84+
/// all state of a LogicalCollection in the coordinator case needs to be derived
85+
/// from the JSON info in the agency's plan entry for the collection...
7886
class LogicalCollection : public LogicalDataSource {
7987
friend struct ::TRI_vocbase_t;
8088

@@ -424,4 +432,4 @@ class LogicalCollection : public LogicalDataSource {
424432

425433
} // namespace arangodb
426434

427-
#endif
435+
#endif

0 commit comments

Comments
 (0)
0