Skip to content

Commit

Permalink
Mark mmaped-files commandline options obsolete (arangodb#18340)
Browse files Browse the repository at this point in the history 
* Mark mmaped-files commandline options obsolete

* Cleanup

* Add a changelog entry
  • Loading branch information
@markuspf
markuspf authored Mar 24, 2023
1 parent 1057b03 commit 7fd66b4
Show file tree
Hide file tree
Showing 11 changed files with 19 additions and 140 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
devel
-----
* Marked all memory-mapping options for Pregel as obsolete;
The memory mapping code was removed as it did not provide any advantages
over spilling into system-provided swap space.

* FE-139 adds new search view type (search-alias).

Expand Down
2 changes: 0 additions & 2 deletions arangod/Pregel/Conductor/Conductor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,6 @@ ErrorCode Conductor::_initializeWorkers() {
.algorithm = std::string{_algorithm->name()},
.userParameters = _specifications.userParameters,
.coordinatorId = coordinatorId,
.useMemoryMaps = _specifications.useMemoryMaps,
.parallelism = _specifications.parallelism,
.edgeCollectionRestrictions =
_specifications.edgeCollectionRestrictions,
Expand Down Expand Up @@ -738,7 +737,6 @@ void Conductor::toVelocyPack(VPackBuilder& result) const {
VPackObjectBuilder ob(&result, "masterContext");
_masterContext->serializeValues(result);
}
result.add("useMemoryMaps", VPackValue(_specifications.useMemoryMaps));

result.add(VPackValue("detail"));
auto conductorStatus = _status.accumulate();
Expand Down
1 change: 0 additions & 1 deletion arangod/Pregel/Conductor/ExecutionStates/CreateWorkersState.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ auto workerSpecification(
.algorithm = std::string{specifications.algorithm},
.userParameters = specifications.userParameters,
.coordinatorId = "",
.useMemoryMaps = specifications.useMemoryMaps,
.parallelism = specifications.parallelism,
.edgeCollectionRestrictions =
specifications.edgeCollectionRestrictions,
Expand Down
130 changes: 16 additions & 114 deletions arangod/Pregel/PregelFeature.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,6 @@ ResultT<ExecutionNumber> PregelFeature::startExecution(TRI_vocbase_t& vocbase,
// set to "infinity"
maxSuperstep = std::numeric_limits<uint64_t>::max();
}
auto useMemoryMapsVar = basics::VelocyPackHelper::getBooleanValue(
options.userParameters.slice(), Utils::useMemoryMapsKey, useMemoryMaps());

auto storeResults = basics::VelocyPackHelper::getBooleanValue(
options.userParameters.slice(), "store", true);

Expand All @@ -352,7 +349,6 @@ ResultT<ExecutionNumber> PregelFeature::startExecution(TRI_vocbase_t& vocbase,
.edgeCollectionRestrictions =
std::move(edgeCollectionRestrictionsPerShard),
.maxSuperstep = maxSuperstep,
.useMemoryMaps = useMemoryMapsVar,
.storeResults = storeResults,
.ttl = ttl,
.parallelism = parallelismVar,
Expand Down Expand Up @@ -413,8 +409,6 @@ PregelFeature::PregelFeature(Server& server)
_defaultParallelism(::defaultParallelism()),
_minParallelism(1),
_maxParallelism(::availableCores()),
_tempLocationType("temp-directory"),
_useMemoryMaps(true),
_softShutdownOngoing(false),
_metrics(std::make_shared<PregelMetrics>(
server.getFeature<metrics::MetricsFeature>())),
Expand Down Expand Up @@ -503,11 +497,10 @@ applies per DB-Server.
Defaults to the number of available cores.)");

options
->addOption(
->addObsoleteOption(
"--pregel.memory-mapped-files",
"Whether to use memory mapped files for storing Pregel "
"temporary data (as opposed to storing it in RAM) by default.",
new BooleanParameter(&_useMemoryMaps),
arangodb::options::makeFlags(
arangodb::options::Flags::DefaultNoComponents,
arangodb::options::Flags::OnCoordinator,
Expand All @@ -527,14 +520,12 @@ You can override this option for each Pregel job by setting the `useMemoryMaps`
attribute of the job.)");

options
->addOption("--pregel.memory-mapped-files-location-type",
"The location for Pregel's temporary files.",
new DiscreteValuesParameter<StringParameter>(
&_tempLocationType, ::tempLocationTypes),
arangodb::options::makeFlags(
arangodb::options::Flags::DefaultNoComponents,
arangodb::options::Flags::OnDBServer,
arangodb::options::Flags::OnSingle))
->addObsoleteOption("--pregel.memory-mapped-files-location-type",
"The location for Pregel's temporary files.",
arangodb::options::makeFlags(
arangodb::options::Flags::DefaultNoComponents,
arangodb::options::Flags::OnDBServer,
arangodb::options::Flags::OnSingle))
.setIntroducedIn(31000)
.setLongDescription(R"(You can configure the location for the
memory-mapped files written by Pregel with this option. This option is only
Expand Down Expand Up @@ -566,36 +557,20 @@ temporary files are stored in there too, it has to provide enough capacity to
store both the regular database data and the Pregel files.)");

options
->addOption("--pregel.memory-mapped-files-custom-path",
"Custom path for Pregel's temporary files. Only used if "
"`--pregel.memory-mapped-files-location` is \"custom\".",
new StringParameter(&_tempLocationCustomPath),
arangodb::options::makeFlags(
arangodb::options::Flags::DefaultNoComponents,
arangodb::options::Flags::OnDBServer,
arangodb::options::Flags::OnSingle))
->addObsoleteOption(
"--pregel.memory-mapped-files-custom-path",
"Custom path for Pregel's temporary files. Only used if "
"`--pregel.memory-mapped-files-location` is \"custom\".",
arangodb::options::makeFlags(
arangodb::options::Flags::DefaultNoComponents,
arangodb::options::Flags::OnDBServer,
arangodb::options::Flags::OnSingle))
.setIntroducedIn(31000)
.setLongDescription(R"(If you use this option, you need to specify the
storage directory location as an absolute path.)");
}

void PregelFeature::validateOptions(std::shared_ptr<ProgramOptions> options) {
if (!_tempLocationCustomPath.empty() && _tempLocationType != "custom") {
LOG_TOPIC("0dd1d", FATAL, Logger::PREGEL)
<< "invalid settings for Pregel's temporary files: if a custom path is "
"provided, "
<< "`--pregel.memory-mapped-files-location-type` must have a value of "
"'custom'";
FATAL_ERROR_EXIT();
} else if (_tempLocationCustomPath.empty() && _tempLocationType == "custom") {
LOG_TOPIC("9b378", FATAL, Logger::PREGEL)
<< "invalid settings for Pregel's temporary files: if "
"`--pregel.memory-mapped-files-location-type` is 'custom', a custom "
"directory must be provided via "
"`--pregel.memory-mapped-files-custom-path`";
FATAL_ERROR_EXIT();
}

if (_minParallelism > _maxParallelism ||
_defaultParallelism < _minParallelism ||
_defaultParallelism > _maxParallelism || _minParallelism == 0 ||
Expand All @@ -614,8 +589,6 @@ void PregelFeature::validateOptions(std::shared_ptr<ProgramOptions> options) {
<< ", default: " << _defaultParallelism;
}

TRI_ASSERT(::tempLocationTypes.contains(_tempLocationType));

// these assertions should always hold
TRI_ASSERT(_minParallelism > 0 && _minParallelism <= _maxParallelism);
TRI_ASSERT(_defaultParallelism > 0 &&
Expand All @@ -624,64 +597,9 @@ void PregelFeature::validateOptions(std::shared_ptr<ProgramOptions> options) {
}

void PregelFeature::start() {
std::string tempDirectory = tempPath();

if (!tempDirectory.empty()) {
TRI_ASSERT(_tempLocationType == "custom" ||
_tempLocationType == "database-directory");

// if the target directory for temporary files does not yet exist, create it
// on the fly! in case we want the temporary files to be created underneath
// the database's data directory, create the directory once. if a custom
// temporary directory was given, we can assume it to be reasonably stable
// across restarts, so it is fine to create it. if we want to store
// temporary files in the temporary directory, we should not create it upon
// startup, simply because the temporary directory can change with every
// instance start.
if (!basics::FileUtils::isDirectory(tempDirectory)) {
std::string systemErrorStr;
long errorNo;

auto res = TRI_CreateRecursiveDirectory(tempDirectory.c_str(), errorNo,
systemErrorStr);

if (res != TRI_ERROR_NO_ERROR) {
LOG_TOPIC("eb2da", FATAL, arangodb::Logger::PREGEL)
<< "unable to create directory for Pregel temporary files '"
<< tempDirectory << "': " << systemErrorStr;
FATAL_ERROR_EXIT();
}
} else {
// temp directory already existed at startup.
// now, if it is underneath the database path, we own it and can
// wipe its contents. if it is not underneath the database path,
// we cannot assume ownership for the files in it and better leave
// the files alone.
if (_tempLocationType == "database-directory") {
auto files = basics::FileUtils::listFiles(tempDirectory);
for (auto const& f : files) {
std::string fqn = basics::FileUtils::buildFilename(tempDirectory, f);

LOG_TOPIC("876fd", INFO, Logger::PREGEL)
<< "removing Pregel temporary file '" << fqn << "' at startup";

ErrorCode res = basics::FileUtils::remove(fqn);

if (res != TRI_ERROR_NO_ERROR) {
LOG_TOPIC("cae59", INFO, Logger::PREGEL)
<< "unable to remove Pregel temporary file '" << fqn
<< "': " << TRI_last_error();
}
}
}
}
}

LOG_TOPIC("a0eb6", DEBUG, Logger::PREGEL)
<< "using Pregel default parallelism " << _defaultParallelism
<< " (min: " << _minParallelism << ", max: " << _maxParallelism << ")"
<< ", memory mapping: " << (_useMemoryMaps ? "on" : "off")
<< ", temp path: " << tempDirectory;
<< " (min: " << _minParallelism << ", max: " << _maxParallelism << ")";

if (!ServerState::instance()->isAgent()) {
scheduleGarbageCollection();
Expand Down Expand Up @@ -739,20 +657,6 @@ bool PregelFeature::isStopping() const noexcept {
return server().isStopping();
}

std::string PregelFeature::tempPath() const {
if (_tempLocationType == "database-directory") {
auto& databasePathFeature = server().getFeature<DatabasePathFeature>();
return databasePathFeature.subdirectoryName("pregel");
}
if (_tempLocationType == "custom") {
TRI_ASSERT(!_tempLocationCustomPath.empty());
return _tempLocationCustomPath;
}

TRI_ASSERT(_tempLocationType == "temp-directory");
return "";
}

size_t PregelFeature::defaultParallelism() const noexcept {
return _defaultParallelism;
}
Expand All @@ -779,8 +683,6 @@ size_t PregelFeature::parallelism(VPackSlice params) const noexcept {
return parallelism;
}

bool PregelFeature::useMemoryMaps() const noexcept { return _useMemoryMaps; }

void PregelFeature::addConductor(std::shared_ptr<Conductor>&& c,
ExecutionNumber executionNumber) {
if (isStopping() || _softShutdownOngoing.load(std::memory_order_relaxed)) {
Expand Down
12 changes: 0 additions & 12 deletions arangod/Pregel/PregelFeature.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ class PregelFeature final : public ArangodFeature {
size_t parallelism(VPackSlice params) const noexcept;

std::string tempPath() const;
bool useMemoryMaps() const noexcept;

auto metrics() -> std::shared_ptr<PregelMetrics> { return _metrics; }

Expand All @@ -134,17 +133,6 @@ class PregelFeature final : public ArangodFeature {
// max parallelism usable per Pregel job
size_t _maxParallelism;

// type of temporary directory location ("custom", "temp-directory",
// "database-directory")
std::string _tempLocationType;

// custom path for temporary directory. only populated if _tempLocationType ==
// "custom"
std::string _tempLocationCustomPath;

// default "useMemoryMaps" value per Pregel job
bool _useMemoryMaps;

mutable Mutex _mutex;

Scheduler::WorkHandle _gcHandle;
Expand Down
2 changes: 0 additions & 2 deletions arangod/Pregel/PregelOptions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ struct ExecutionSpecifications {
/// and used in MasterContext::postGlobalSuperstep which returns whether to
/// continue.
uint64_t maxSuperstep;
bool useMemoryMaps;
bool storeResults;
TTL ttl;
size_t parallelism;
Expand All @@ -151,7 +150,6 @@ auto inspect(Inspector& f, ExecutionSpecifications& x) {
f.field("edgeCollections", x.edgeCollections),
f.field("edgeCollectionRestrictions", x.edgeCollectionRestrictions),
f.field("maxSuperstep", x.maxSuperstep),
f.field("useMemoryMaps", x.useMemoryMaps),
f.field("storeResults", x.storeResults), f.field("ttl", x.ttl),
f.field("parallelism", x.parallelism),
f.field("userParamters", x.userParameters));
Expand Down
1 change: 0 additions & 1 deletion arangod/Pregel/Utils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ std::string const Utils::vertexShardsKey = "vertexShards";
std::string const Utils::edgeShardsKey = "edgeShards";
std::string const Utils::globalShardListKey = "globalShardList";
std::string const Utils::userParametersKey = "userparams";
std::string const Utils::useMemoryMapsKey = "useMemoryMaps";
std::string const Utils::parallelismKey = "parallelism";

std::string const Utils::globalSuperstepKey = "gss";
Expand Down
1 change: 0 additions & 1 deletion arangod/Pregel/Utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ class Utils {
static std::string const edgeShardsKey;
static std::string const globalShardListKey;
static std::string const userParametersKey;
static std::string const useMemoryMapsKey;
static std::string const parallelismKey;

/// Current global superstep
Expand Down
2 changes: 0 additions & 2 deletions arangod/Pregel/Worker/Messages.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ struct CreateWorker {
std::string algorithm;
VPackBuilder userParameters;
std::string coordinatorId;
bool useMemoryMaps;
size_t parallelism;
std::unordered_map<CollectionID, std::vector<ShardID>>
edgeCollectionRestrictions;
Expand All @@ -58,7 +57,6 @@ auto inspect(Inspector& f, CreateWorker& x) {
f.field("algorithm", x.algorithm),
f.field("userParameters", x.userParameters),
f.field("coordinatorId", x.coordinatorId),
f.field("useMemoryMaps", x.useMemoryMaps),
f.field("parallelism", x.parallelism),
f.field("edgeCollectionRestrictions", x.edgeCollectionRestrictions),
f.field("vertexShards", x.vertexShards),
Expand Down
1 change: 0 additions & 1 deletion arangod/Pregel/Worker/WorkerConfig.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ std::string const& WorkerConfig::database() const { return _vocbase->name(); }
void WorkerConfig::updateConfig(worker::message::CreateWorker const& params) {
_executionNumber = params.executionNumber;
_coordinatorId = params.coordinatorId;
_useMemoryMaps = params.useMemoryMaps;
_globalShardIDs = params.allShards;

_parallelism = params.parallelism;
Expand Down
4 changes: 0 additions & 4 deletions arangod/Pregel/Worker/WorkerConfig.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ class WorkerConfig : std::enable_shared_from_this<WorkerConfig> {

inline uint64_t localSuperstep() const { return _localSuperstep; }

inline bool useMemoryMaps() const { return _useMemoryMaps; }

inline uint64_t parallelism() const { return _parallelism; }

inline std::string const& coordinatorId() const { return _coordinatorId; }
Expand Down Expand Up @@ -146,8 +144,6 @@ class WorkerConfig : std::enable_shared_from_this<WorkerConfig> {
std::string _coordinatorId;
TRI_vocbase_t* _vocbase;

// use memory mapping? will be updated by config later
bool _useMemoryMaps = true;
// parallelism. will be updated by config later
size_t _parallelism = 1;

Expand Down

0 comments on commit 7fd66b4

Please sign in to comment.