You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by mp...@apache.org on 2018/01/11 00:18:27 UTC
[2/5] mesos git commit: Updated uses of `protobuf::read(path)` which
now returns `Try`.
Updated uses of `protobuf::read(path)` which now returns `Try<T>`.
Since the path version of `protobuf::read` now returns `Try<T>`,
many of the existing code is removed and/or simplified.
Review: https://reviews.apache.org/r/65022
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/fda054b5
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/fda054b5
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/fda054b5
Branch: refs/heads/master
Commit: fda054b50ff7cdd2d7a60d31cfe24ce42bfbfaa5
Parents: 4f9cda1
Author: Michael Park <mp...@apache.org>
Authored: Fri Jan 5 17:44:41 2018 -0800
Committer: Michael Park <mp...@apache.org>
Committed: Wed Jan 10 15:57:10 2018 -0800
----------------------------------------------------------------------
src/resource_provider/storage/provider.cpp | 160 +++++++++----------
src/slave/containerizer/mesos/paths.cpp | 30 ++--
.../provisioner/docker/metadata_manager.cpp | 8 +-
.../mesos/provisioner/provisioner.cpp | 20 ++-
src/slave/state.cpp | 38 +----
src/tests/protobuf_io_tests.cpp | 9 +-
src/tests/slave_recovery_tests.cpp | 6 +-
7 files changed, 111 insertions(+), 160 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/fda054b5/src/resource_provider/storage/provider.cpp
----------------------------------------------------------------------
diff --git a/src/resource_provider/storage/provider.cpp b/src/resource_provider/storage/provider.cpp
index 55f2e66..c0bf3fa 100644
--- a/src/resource_provider/storage/provider.cpp
+++ b/src/resource_provider/storage/provider.cpp
@@ -705,7 +705,7 @@ Future<Nothing> StorageLocalResourceProviderProcess::recoverServices()
containerId);
if (os::exists(configPath)) {
- Result<CSIPluginContainerInfo> config =
+ Try<CSIPluginContainerInfo> config =
::protobuf::read<CSIPluginContainerInfo>(configPath);
if (config.isError()) {
@@ -714,13 +714,11 @@ Future<Nothing> StorageLocalResourceProviderProcess::recoverServices()
configPath + "': " + config.error());
}
- if (config.isSome()) {
- convertResourceFormat(
- config->mutable_resources(), POST_RESERVATION_REFINEMENT);
+ convertResourceFormat(
+ config->mutable_resources(), POST_RESERVATION_REFINEMENT);
- if (getCSIPluginContainerInfo(info, containerId) == config.get()) {
- continue;
- }
+ if (getCSIPluginContainerInfo(info, containerId) == config.get()) {
+ continue;
}
}
}
@@ -800,7 +798,7 @@ Future<Nothing> StorageLocalResourceProviderProcess::recoverVolumes()
continue;
}
- Result<csi::state::VolumeState> volumeState =
+ Try<csi::state::VolumeState> volumeState =
::protobuf::read<csi::state::VolumeState>(statePath);
if (volumeState.isError()) {
@@ -809,69 +807,67 @@ Future<Nothing> StorageLocalResourceProviderProcess::recoverVolumes()
volumeState.error());
}
- if (volumeState.isSome()) {
- volumes.put(volumeId, std::move(volumeState.get()));
-
- Future<Nothing> recovered = Nothing();
+ volumes.put(volumeId, std::move(volumeState.get()));
- switch (volumes.at(volumeId).state.state()) {
- case csi::state::VolumeState::CREATED:
- case csi::state::VolumeState::NODE_READY: {
- break;
- }
- case csi::state::VolumeState::PUBLISHED: {
- if (volumes.at(volumeId).state.boot_id() != bootId) {
- // The node has been restarted since the volume is mounted,
- // so it is no longer in the `PUBLISHED` state.
- volumes.at(volumeId).state.set_state(
- csi::state::VolumeState::NODE_READY);
- volumes.at(volumeId).state.clear_boot_id();
- checkpointVolumeState(volumeId);
- }
- break;
- }
- case csi::state::VolumeState::CONTROLLER_PUBLISH: {
- recovered =
- volumes.at(volumeId).sequence->add(std::function<Future<Nothing>()>(
- defer(self(), &Self::controllerPublish, volumeId)));
- break;
- }
- case csi::state::VolumeState::CONTROLLER_UNPUBLISH: {
- recovered =
- volumes.at(volumeId).sequence->add(std::function<Future<Nothing>()>(
- defer(self(), &Self::controllerUnpublish, volumeId)));
- break;
- }
- case csi::state::VolumeState::NODE_PUBLISH: {
- recovered =
- volumes.at(volumeId).sequence->add(std::function<Future<Nothing>()>(
- defer(self(), &Self::nodePublish, volumeId)));
- break;
- }
- case csi::state::VolumeState::NODE_UNPUBLISH: {
- recovered =
- volumes.at(volumeId).sequence->add(std::function<Future<Nothing>()>(
- defer(self(), &Self::nodeUnpublish, volumeId)));
- break;
- }
- case csi::state::VolumeState::UNKNOWN: {
- recovered = Failure(
- "Volume '" + volumeId + "' is in " +
- stringify(volumes.at(volumeId).state.state()) + " state");
- }
+ Future<Nothing> recovered = Nothing();
- // NOTE: We avoid using a default clause for the following
- // values in proto3's open enum to enable the compiler to detect
- // missing enum cases for us. See:
- // https://github.com/google/protobuf/issues/3917
- case google::protobuf::kint32min:
- case google::protobuf::kint32max: {
- UNREACHABLE();
+ switch (volumes.at(volumeId).state.state()) {
+ case csi::state::VolumeState::CREATED:
+ case csi::state::VolumeState::NODE_READY: {
+ break;
+ }
+ case csi::state::VolumeState::PUBLISHED: {
+ if (volumes.at(volumeId).state.boot_id() != bootId) {
+ // The node has been restarted since the volume is mounted,
+ // so it is no longer in the `PUBLISHED` state.
+ volumes.at(volumeId).state.set_state(
+ csi::state::VolumeState::NODE_READY);
+ volumes.at(volumeId).state.clear_boot_id();
+ checkpointVolumeState(volumeId);
}
+ break;
+ }
+ case csi::state::VolumeState::CONTROLLER_PUBLISH: {
+ recovered =
+ volumes.at(volumeId).sequence->add(std::function<Future<Nothing>()>(
+ defer(self(), &Self::controllerPublish, volumeId)));
+ break;
+ }
+ case csi::state::VolumeState::CONTROLLER_UNPUBLISH: {
+ recovered =
+ volumes.at(volumeId).sequence->add(std::function<Future<Nothing>()>(
+ defer(self(), &Self::controllerUnpublish, volumeId)));
+ break;
+ }
+ case csi::state::VolumeState::NODE_PUBLISH: {
+ recovered =
+ volumes.at(volumeId).sequence->add(std::function<Future<Nothing>()>(
+ defer(self(), &Self::nodePublish, volumeId)));
+ break;
+ }
+ case csi::state::VolumeState::NODE_UNPUBLISH: {
+ recovered =
+ volumes.at(volumeId).sequence->add(std::function<Future<Nothing>()>(
+ defer(self(), &Self::nodeUnpublish, volumeId)));
+ break;
+ }
+ case csi::state::VolumeState::UNKNOWN: {
+ recovered = Failure(
+ "Volume '" + volumeId + "' is in " +
+ stringify(volumes.at(volumeId).state.state()) + " state");
}
- futures.push_back(recovered);
+ // NOTE: We avoid using a default clause for the following
+ // values in proto3's open enum to enable the compiler to detect
+ // missing enum cases for us. See:
+ // https://github.com/google/protobuf/issues/3917
+ case google::protobuf::kint32min:
+ case google::protobuf::kint32max: {
+ UNREACHABLE();
+ }
}
+
+ futures.push_back(recovered);
}
return collect(futures).then([] { return Nothing(); });
@@ -906,7 +902,7 @@ StorageLocalResourceProviderProcess::recoverResourceProviderState()
return Nothing();
}
- Result<ResourceProviderState> resourceProviderState =
+ Try<ResourceProviderState> resourceProviderState =
::protobuf::read<ResourceProviderState>(statePath);
if (resourceProviderState.isError()) {
@@ -915,28 +911,26 @@ StorageLocalResourceProviderProcess::recoverResourceProviderState()
"': " + resourceProviderState.error());
}
- if (resourceProviderState.isSome()) {
- foreach (
- Operation& operation,
- *resourceProviderState->mutable_operations()) {
- upgradeResources(operation.mutable_info());
- }
+ foreach (
+ Operation& operation,
+ *resourceProviderState->mutable_operations()) {
+ upgradeResources(operation.mutable_info());
+ }
- convertResourceFormat(
- resourceProviderState->mutable_resources(),
- POST_RESERVATION_REFINEMENT);
+ convertResourceFormat(
+ resourceProviderState->mutable_resources(),
+ POST_RESERVATION_REFINEMENT);
- foreach (const Operation& operation,
- resourceProviderState->operations()) {
- Try<id::UUID> uuid = id::UUID::fromBytes(operation.uuid().value());
+ foreach (const Operation& operation,
+ resourceProviderState->operations()) {
+ Try<id::UUID> uuid = id::UUID::fromBytes(operation.uuid().value());
- CHECK_SOME(uuid);
+ CHECK_SOME(uuid);
- operations[uuid.get()] = operation;
- }
-
- totalResources = resourceProviderState->resources();
+ operations[uuid.get()] = operation;
}
+
+ totalResources = resourceProviderState->resources();
}
return Nothing();
http://git-wip-us.apache.org/repos/asf/mesos/blob/fda054b5/src/slave/containerizer/mesos/paths.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/paths.cpp b/src/slave/containerizer/mesos/paths.cpp
index d6ea618..4cec2d2 100644
--- a/src/slave/containerizer/mesos/paths.cpp
+++ b/src/slave/containerizer/mesos/paths.cpp
@@ -276,7 +276,7 @@ Result<ContainerTermination> getContainerTermination(
return None();
}
- Result<ContainerTermination> termination =
+ Try<ContainerTermination> termination =
::protobuf::read<ContainerTermination>(path);
if (termination.isError()) {
@@ -284,10 +284,8 @@ Result<ContainerTermination> getContainerTermination(
termination.error());
}
- if (termination.isSome()) {
- convertResourceFormat(
- termination->mutable_limited_resources(), POST_RESERVATION_REFINEMENT);
- }
+ convertResourceFormat(
+ termination->mutable_limited_resources(), POST_RESERVATION_REFINEMENT);
return termination;
}
@@ -329,7 +327,7 @@ Result<ContainerConfig> getContainerConfig(
return None();
}
- Result<ContainerConfig> containerConfig =
+ Try<ContainerConfig> containerConfig =
::protobuf::read<ContainerConfig>(path);
if (containerConfig.isError()) {
@@ -337,18 +335,16 @@ Result<ContainerConfig> getContainerConfig(
containerConfig.error());
}
- if (containerConfig.isSome()) {
- convertResourceFormat(
- containerConfig->mutable_executor_info()->mutable_resources(),
- POST_RESERVATION_REFINEMENT);
+ convertResourceFormat(
+ containerConfig->mutable_executor_info()->mutable_resources(),
+ POST_RESERVATION_REFINEMENT);
- convertResourceFormat(
- containerConfig->mutable_task_info()->mutable_resources(),
- POST_RESERVATION_REFINEMENT);
+ convertResourceFormat(
+ containerConfig->mutable_task_info()->mutable_resources(),
+ POST_RESERVATION_REFINEMENT);
- convertResourceFormat(
- containerConfig->mutable_resources(), POST_RESERVATION_REFINEMENT);
- }
+ convertResourceFormat(
+ containerConfig->mutable_resources(), POST_RESERVATION_REFINEMENT);
return containerConfig;
}
@@ -439,7 +435,7 @@ Result<ContainerLaunchInfo> getContainerLaunchInfo(
return None();
}
- const Result<ContainerLaunchInfo>& containerLaunchInfo =
+ Try<ContainerLaunchInfo> containerLaunchInfo =
::protobuf::read<ContainerLaunchInfo>(path);
if (containerLaunchInfo.isError()) {
http://git-wip-us.apache.org/repos/asf/mesos/blob/fda054b5/src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp b/src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp
index 1ab66c1..548c524 100644
--- a/src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp
+++ b/src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp
@@ -256,18 +256,12 @@ Future<Nothing> MetadataManagerProcess::recover()
return Nothing();
}
- Result<Images> images = ::protobuf::read<Images>(storedImagesPath);
+ Try<Images> images = ::protobuf::read<Images>(storedImagesPath);
if (images.isError()) {
return Failure("Failed to read images from '" + storedImagesPath + "' " +
images.error());
}
- if (images.isNone()) {
- // This could happen if the slave died after opening the file for
- // writing but before persisted on disk.
- return Failure("Unexpected empty images file '" + storedImagesPath + "'");
- }
-
foreach (const Image& image, images.get().images()) {
const string imageReference = stringify(image.reference());
http://git-wip-us.apache.org/repos/asf/mesos/blob/fda054b5/src/slave/containerizer/mesos/provisioner/provisioner.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/provisioner/provisioner.cpp b/src/slave/containerizer/mesos/provisioner/provisioner.cpp
index 7621c49..57a437b 100644
--- a/src/slave/containerizer/mesos/provisioner/provisioner.cpp
+++ b/src/slave/containerizer/mesos/provisioner/provisioner.cpp
@@ -420,7 +420,6 @@ Future<Nothing> ProvisionerProcess::recover(
info->rootfses.put(backend, rootfses.get()[backend]);
}
- Result<ContainerLayers> layers = None();
const string path = provisioner::paths::getLayersFilePath(
rootDir, containerId);
@@ -430,19 +429,18 @@ Future<Nothing> ProvisionerProcess::recover(
VLOG(1) << "Layers path '" << path << "' is missing for container' "
<< containerId << "'";
} else {
- layers = ::protobuf::read<ContainerLayers>(path);
- }
+ Try<ContainerLayers> layers = ::protobuf::read<ContainerLayers>(path);
+ if (layers.isError()) {
+ return Failure(
+ "Failed to recover layers for container '" +
+ stringify(containerId) + "': " + layers.error());
+ }
- if (layers.isError()) {
- return Failure(
- "Failed to recover layers for container '" + stringify(containerId) +
- "': " + layers.error());
- } else if (layers.isSome()) {
info->layers = vector<string>();
std::copy(
- layers->paths().begin(),
- layers->paths().end(),
- std::back_inserter(info->layers.get()));
+ layers->paths().begin(),
+ layers->paths().end(),
+ std::back_inserter(info->layers.get()));
}
infos.put(containerId, info);
http://git-wip-us.apache.org/repos/asf/mesos/blob/fda054b5/src/slave/state.cpp
----------------------------------------------------------------------
diff --git a/src/slave/state.cpp b/src/slave/state.cpp
index 5428b34..0bc0cca 100644
--- a/src/slave/state.cpp
+++ b/src/slave/state.cpp
@@ -151,8 +151,7 @@ Try<SlaveState> SlaveState::recover(
return state;
}
- Result<SlaveInfo> slaveInfo = ::protobuf::read<SlaveInfo>(path);
-
+ Try<SlaveInfo> slaveInfo = ::protobuf::read<SlaveInfo>(path);
if (slaveInfo.isError()) {
const string& message = "Failed to read agent info from '" + path + "': " +
slaveInfo.error();
@@ -165,13 +164,6 @@ Try<SlaveState> SlaveState::recover(
}
}
- if (slaveInfo.isNone()) {
- // This could happen if the slave died after opening the file for
- // writing but before it checkpointed anything.
- LOG(WARNING) << "Found empty agent info file '" << path << "'";
- return state;
- }
-
convertResourceFormat(
slaveInfo.get().mutable_resources(),
POST_RESERVATION_REFINEMENT);
@@ -227,7 +219,7 @@ Try<FrameworkState> FrameworkState::recover(
return state;
}
- const Result<FrameworkInfo>& frameworkInfo =
+ const Try<FrameworkInfo> frameworkInfo =
::protobuf::read<FrameworkInfo>(path);
if (frameworkInfo.isError()) {
@@ -243,13 +235,6 @@ Try<FrameworkState> FrameworkState::recover(
}
}
- if (frameworkInfo.isNone()) {
- // This could happen if the slave died after opening the file for
- // writing but before it checkpointed anything.
- LOG(WARNING) << "Found empty framework info file '" << path << "'";
- return state;
- }
-
state.info = frameworkInfo.get();
// Read the framework pid.
@@ -394,8 +379,7 @@ Try<ExecutorState> ExecutorState::recover(
return state;
}
- Result<ExecutorInfo> executorInfo = ::protobuf::read<ExecutorInfo>(path);
-
+ Try<ExecutorInfo> executorInfo = ::protobuf::read<ExecutorInfo>(path);
if (executorInfo.isError()) {
message = "Failed to read executor info from '" + path + "': " +
executorInfo.error();
@@ -409,13 +393,6 @@ Try<ExecutorState> ExecutorState::recover(
}
}
- if (executorInfo.isNone()) {
- // This could happen if the slave died after opening the file for
- // writing but before it checkpointed anything.
- LOG(WARNING) << "Found empty executor info file '" << path << "'";
- return state;
- }
-
convertResourceFormat(
executorInfo.get().mutable_resources(),
POST_RESERVATION_REFINEMENT);
@@ -595,7 +572,7 @@ Try<TaskState> TaskState::recover(
return state;
}
- Result<Task> task = ::protobuf::read<Task>(path);
+ Try<Task> task = ::protobuf::read<Task>(path);
if (task.isError()) {
message = "Failed to read task info from '" + path + "': " + task.error();
@@ -609,13 +586,6 @@ Try<TaskState> TaskState::recover(
}
}
- if (task.isNone()) {
- // This could happen if the slave died after opening the file for
- // writing but before it checkpointed anything.
- LOG(WARNING) << "Found empty task info file '" << path << "'";
- return state;
- }
-
convertResourceFormat(
task.get().mutable_resources(),
POST_RESERVATION_REFINEMENT);
http://git-wip-us.apache.org/repos/asf/mesos/blob/fda054b5/src/tests/protobuf_io_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/protobuf_io_tests.cpp b/src/tests/protobuf_io_tests.cpp
index 4a2e3a3..ddbda03 100644
--- a/src/tests/protobuf_io_tests.cpp
+++ b/src/tests/protobuf_io_tests.cpp
@@ -150,15 +150,14 @@ TEST_F(ProtobufIOTest, RepeatedPtrField)
Try<Nothing> write = ::protobuf::write(file, expected);
ASSERT_SOME(write);
- Result<RepeatedPtrField<FrameworkID>> read =
+ Try<RepeatedPtrField<FrameworkID>> actual =
::protobuf::read<RepeatedPtrField<FrameworkID>>(file);
- ASSERT_SOME(read);
- RepeatedPtrField<FrameworkID> actual = read.get();
+ ASSERT_SOME(actual);
- ASSERT_EQ(expected.size(), actual.size());
+ ASSERT_EQ(expected.size(), actual->size());
for (size_t i = 0; i < size; i++) {
- EXPECT_EQ(expected.Get(i), actual.Get(i));
+ EXPECT_EQ(expected.Get(i), actual->Get(i));
}
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/fda054b5/src/tests/slave_recovery_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/slave_recovery_tests.cpp b/src/tests/slave_recovery_tests.cpp
index e305d74..fc26987 100644
--- a/src/tests/slave_recovery_tests.cpp
+++ b/src/tests/slave_recovery_tests.cpp
@@ -125,10 +125,10 @@ TEST_F(SlaveStateTest, CheckpointProtobufMessage)
SlaveID expected;
expected.set_value("agent1");
- const string& file = "slave.id";
+ const string file = "slave.id";
slave::state::checkpoint(file, expected);
- const Result<SlaveID>& actual = ::protobuf::read<SlaveID>(file);
+ const Try<SlaveID> actual = ::protobuf::read<SlaveID>(file);
ASSERT_SOME(actual);
EXPECT_SOME_EQ(expected, actual);
@@ -144,7 +144,7 @@ TEST_F(SlaveStateTest, CheckpointRepeatedProtobufMessages)
const string file = "resources-file";
slave::state::checkpoint(file, expected);
- Result<RepeatedPtrField<Resource>> actual =
+ Try<RepeatedPtrField<Resource>> actual =
::protobuf::read<RepeatedPtrField<Resource>>(file);
ASSERT_SOME(actual);