CommonApi: Remove template parameter from UploadAndUpdateContainer

...since it works with ArtifactBlobs only.

9 files changed, 81 insertions, 80 deletions

diff --git a/src/buildtool/execution_api/common/TARGETS b/src/buildtool/execution_api/common/TARGETS
index df50ecaf..2687aec7 100644
--- a/src/buildtool/execution_api/common/TARGETS
+++ b/src/buildtool/execution_api/common/TARGETS
@@ -90,21 +90,21 @@
   , "hdrs": ["common_api.hpp"]
   , "srcs": ["common_api.cpp"]
   , "deps":
-    [ "blob_tree"
+    [ "artifact_blob_container"
+    , "blob_tree"
     , "common"
-    , "content_blob_container"
-    , "message_limits"
     , ["@", "gsl", "", "gsl"]
     , ["src/buildtool/common", "common"]
     , ["src/buildtool/execution_api/bazel_msg", "bazel_msg_factory"]
     , ["src/buildtool/execution_api/bazel_msg", "directory_tree"]
-    , ["src/buildtool/logging", "log_level"]
     , ["src/buildtool/logging", "logging"]
     ]
   , "stage": ["src", "buildtool", "execution_api", "common"]
   , "private-deps":
-    [ "artifact_blob_container"
+    [ "content_blob_container"
+    , "message_limits"
     , ["@", "fmt", "", "fmt"]
+    , ["src/buildtool/logging", "log_level"]
     , ["src/buildtool/file_system", "object_type"]
     ]
   }
diff --git a/src/buildtool/execution_api/common/common_api.cpp b/src/buildtool/execution_api/common/common_api.cpp
index 8bd8f0e0..3a713fc4 100644
--- a/src/buildtool/execution_api/common/common_api.cpp
+++ b/src/buildtool/execution_api/common/common_api.cpp
@@ -28,8 +28,10 @@
 #include <utility>
 
 #include "fmt/core.h"
-#include "src/buildtool/execution_api/common/artifact_blob_container.hpp"
+#include "src/buildtool/execution_api/common/content_blob_container.hpp"
+#include "src/buildtool/execution_api/common/message_limits.hpp"
 #include "src/buildtool/file_system/object_type.hpp"
+#include "src/buildtool/logging/log_level.hpp"
 
 auto CommonRetrieveToFds(
     std::vector<Artifact::ObjectInfo> const& artifacts_info,
@@ -123,7 +125,7 @@ auto CommonUploadBlobTree(BlobTreePtr const& blob_tree,
             }
             // Optimize store & upload by taking into account the maximum
             // transfer size.
-            if (not UpdateContainerAndUpload<ArtifactDigest>(
+            if (not UpdateContainerAndUpload(
                     &container,
                     node->Blob(),
                     /*exception_is_fatal=*/false,
@@ -148,7 +150,7 @@ auto CommonUploadTreeCompatible(
     // Store and upload blobs, taking into account the maximum transfer size.
     auto digest = BazelMsgFactory::CreateDirectoryDigestFromTree(
         build_root, resolve_links, [&blobs, &api](ArtifactBlob&& blob) {
-            return UpdateContainerAndUpload<ArtifactDigest>(
+            return UpdateContainerAndUpload(
                 &blobs,
                 std::move(blob),
                 /*exception_is_fatal=*/false,
@@ -202,3 +204,54 @@ auto CommonUploadTreeNative(IExecutionApi const& api,
     }
     return tree_blob.digest;
 }
+
+auto UpdateContainerAndUpload(
+    gsl::not_null<std::unordered_set<ArtifactBlob>*> const& container,
+    ArtifactBlob&& blob,
+    bool exception_is_fatal,
+    std::function<bool(std::unordered_set<ArtifactBlob>&&)> const& uploader,
+    Logger const* logger) noexcept -> bool {
+    // Optimize upload of blobs with respect to the maximum transfer limit, such
+    // that we never store unnecessarily more data in the container than we need
+    // per remote transfer.
+    try {
+        if (blob.data->size() > kMaxBatchTransferSize) {
+            // large blobs use individual stream upload
+            if (not uploader(
+                    std::unordered_set<ArtifactBlob>{{std::move(blob)}})) {
+                return false;
+            }
+        }
+        else {
+            if (not container->contains(blob)) {
+                std::size_t content_size = 0;
+                for (auto const& blob : *container) {
+                    content_size += blob.data->size();
+                }
+
+                if (content_size + blob.data->size() > kMaxBatchTransferSize) {
+                    // swap away from original container to allow move during
+                    // upload
+                    std::unordered_set<ArtifactBlob> tmp_container{};
+                    std::swap(*container, tmp_container);
+                    // if we would surpass the transfer limit, upload the
+                    // current container and clear it before adding more blobs
+                    if (not uploader(std::move(tmp_container))) {
+                        return false;
+                    }
+                }
+                // add current blob to container
+                container->emplace(std::move(blob));
+            }
+        }
+    } catch (std::exception const& ex) {
+        if (exception_is_fatal) {
+            Logger::Log(logger,
+                        LogLevel::Error,
+                        "failed to emplace blob with\n:{}",
+                        ex.what());
+        }
+        return false;
+    }
+    return true;  // success!
+}
diff --git a/src/buildtool/execution_api/common/common_api.hpp b/src/buildtool/execution_api/common/common_api.hpp
index 5060c086..04997b5f 100644
--- a/src/buildtool/execution_api/common/common_api.hpp
+++ b/src/buildtool/execution_api/common/common_api.hpp
@@ -15,9 +15,7 @@
 #ifndef INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_COMMON_API_HPP
 #define INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_COMMON_API_HPP
 
-#include <cstddef>
 #include <cstdio>
-#include <exception>
 #include <functional>
 #include <iterator>
 #include <optional>
@@ -30,11 +28,9 @@
 #include "src/buildtool/common/artifact_digest.hpp"
 #include "src/buildtool/execution_api/bazel_msg/bazel_msg_factory.hpp"
 #include "src/buildtool/execution_api/bazel_msg/directory_tree.hpp"
+#include "src/buildtool/execution_api/common/artifact_blob_container.hpp"
 #include "src/buildtool/execution_api/common/blob_tree.hpp"
-#include "src/buildtool/execution_api/common/content_blob_container.hpp"
 #include "src/buildtool/execution_api/common/execution_api.hpp"
-#include "src/buildtool/execution_api/common/message_limits.hpp"
-#include "src/buildtool/logging/log_level.hpp"
 #include "src/buildtool/logging/logger.hpp"
 
 /// \brief Stores a list of missing artifact digests, as well as a back-mapping
@@ -121,57 +117,11 @@ template <typename TValue, typename TIterator>
 /// \param logger Use this instance for any logging. If nullptr, use the default
 /// logger. This value is used only if exception_is_fatal==true.
 /// \returns Returns true on success, false otherwise (failures or exceptions).
-template <typename TDigest>
-auto UpdateContainerAndUpload(
-    gsl::not_null<std::unordered_set<ContentBlob<TDigest>>*> const& container,
-    ContentBlob<TDigest>&& blob,
+[[nodiscard]] auto UpdateContainerAndUpload(
+    gsl::not_null<std::unordered_set<ArtifactBlob>*> const& container,
+    ArtifactBlob&& blob,
     bool exception_is_fatal,
-    std::function<bool(std::unordered_set<ContentBlob<TDigest>>&&)> const&
-        uploader,
-    Logger const* logger = nullptr) noexcept -> bool {
-    // Optimize upload of blobs with respect to the maximum transfer limit, such
-    // that we never store unnecessarily more data in the container than we need
-    // per remote transfer.
-    try {
-        if (blob.data->size() > kMaxBatchTransferSize) {
-            // large blobs use individual stream upload
-            if (not uploader(std::unordered_set<ContentBlob<TDigest>>{
-                    {std::move(blob)}})) {
-                return false;
-            }
-        }
-        else {
-            if (not container->contains(blob)) {
-                std::size_t content_size = 0;
-                for (auto const& blob : *container) {
-                    content_size += blob.data->size();
-                }
-
-                if (content_size + blob.data->size() > kMaxBatchTransferSize) {
-                    // swap away from original container to allow move during
-                    // upload
-                    std::unordered_set<ContentBlob<TDigest>> tmp_container{};
-                    std::swap(*container, tmp_container);
-                    // if we would surpass the transfer limit, upload the
-                    // current container and clear it before adding more blobs
-                    if (not uploader(std::move(tmp_container))) {
-                        return false;
-                    }
-                }
-                // add current blob to container
-                container->emplace(std::move(blob));
-            }
-        }
-    } catch (std::exception const& ex) {
-        if (exception_is_fatal) {
-            Logger::Log(logger,
-                        LogLevel::Error,
-                        "failed to emplace blob with\n:{}",
-                        ex.what());
-        }
-        return false;
-    }
-    return true;  // success!
-}
+    std::function<bool(std::unordered_set<ArtifactBlob>&&)> const& uploader,
+    Logger const* logger = nullptr) noexcept -> bool;
 
 #endif  // INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_COMMON_API_HPP
diff --git a/src/buildtool/execution_api/git/git_api.hpp b/src/buildtool/execution_api/git/git_api.hpp
index 55bdaabe..5bb891ef 100644
--- a/src/buildtool/execution_api/git/git_api.hpp
+++ b/src/buildtool/execution_api/git/git_api.hpp
@@ -248,7 +248,7 @@ class GitApi final : public IExecutionApi {
                                 hash_function, *entry_content);
                         // Collect blob and upload to remote CAS if transfer
                         // size reached.
-                        if (not UpdateContainerAndUpload<ArtifactDigest>(
+                        if (not UpdateContainerAndUpload(
                                 &tree_deps_only_blobs,
                                 ArtifactBlob{std::move(digest),
                                              *entry_content,
@@ -283,7 +283,7 @@ class GitApi final : public IExecutionApi {
                           hash_function, *content);
 
             // Collect blob and upload to remote CAS if transfer size reached.
-            if (not UpdateContainerAndUpload<ArtifactDigest>(
+            if (not UpdateContainerAndUpload(
                     &container,
                     ArtifactBlob{std::move(digest),
                                  std::move(*content),
diff --git a/src/buildtool/execution_api/local/local_api.hpp b/src/buildtool/execution_api/local/local_api.hpp
index c8877c52..5640b441 100644
--- a/src/buildtool/execution_api/local/local_api.hpp
+++ b/src/buildtool/execution_api/local/local_api.hpp
@@ -206,7 +206,7 @@ class LocalApi final : public IExecutionApi {
                           *content);
 
             // Collect blob and upload to remote CAS if transfer size reached.
-            if (not UpdateContainerAndUpload<ArtifactDigest>(
+            if (not UpdateContainerAndUpload(
                     &container,
                     ArtifactBlob{std::move(digest),
                                  *content,
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp
index d9990f9a..03d79199 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp
@@ -74,7 +74,7 @@ namespace {
                     ? IsExecutableObject(info_map.at(blob.digest).type)
                     : false;
             // Collect blob and upload to other CAS if transfer size reached.
-            if (not UpdateContainerAndUpload<ArtifactDigest>(
+            if (not UpdateContainerAndUpload(
                     &container,
                     std::move(blob),
                     /*exception_is_fatal=*/true,
diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
index e4bc9248..19e7b972 100644
--- a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
+++ b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp
@@ -294,11 +294,10 @@ auto BazelResponse::UploadTreeMessageDirectories(
     }
     auto const root_digest = rootdir_blob->digest;
     // store or upload rootdir blob, taking maximum transfer size into account
-    if (not UpdateContainerAndUpload<ArtifactDigest>(
-            &dir_blobs,
-            *std::move(rootdir_blob),
-            /*exception_is_fatal=*/false,
-            upload_callback)) {
+    if (not UpdateContainerAndUpload(&dir_blobs,
+                                     *std::move(rootdir_blob),
+                                     /*exception_is_fatal=*/false,
+                                     upload_callback)) {
         return unexpected{fmt::format(
             "failed to upload Tree with root digest {}", root_digest.hash())};
     }
@@ -310,11 +309,10 @@ auto BazelResponse::UploadTreeMessageDirectories(
             return unexpected{std::move(blob).error()};
         }
         auto const blob_digest = blob->digest;
-        if (not UpdateContainerAndUpload<ArtifactDigest>(
-                &dir_blobs,
-                *std::move(blob),
-                /*exception_is_fatal=*/false,
-                upload_callback)) {
+        if (not UpdateContainerAndUpload(&dir_blobs,
+                                         *std::move(blob),
+                                         /*exception_is_fatal=*/false,
+                                         upload_callback)) {
             return unexpected{
                 fmt::format("failed to upload Tree subdir with digest {}",
                             blob_digest.hash())};
diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp
index 32186764..efec69c4 100644
--- a/src/buildtool/execution_engine/executor/executor.hpp
+++ b/src/buildtool/execution_engine/executor/executor.hpp
@@ -345,7 +345,7 @@ class ExecutorImpl {
             }
             // store and/or upload blob, taking into account the maximum
             // transfer size
-            if (not UpdateContainerAndUpload<ArtifactDigest>(
+            if (not UpdateContainerAndUpload(
                     &container,
                     ArtifactBlob{*digest,
                                  std::move(*content),
diff --git a/src/buildtool/graph_traverser/graph_traverser.hpp b/src/buildtool/graph_traverser/graph_traverser.hpp
index 28889f38..b35db290 100644
--- a/src/buildtool/graph_traverser/graph_traverser.hpp
+++ b/src/buildtool/graph_traverser/graph_traverser.hpp
@@ -324,7 +324,7 @@ class GraphTraverser {
             });
             // Store and/or upload blob, taking into account the maximum
             // transfer size.
-            if (not UpdateContainerAndUpload<ArtifactDigest>(
+            if (not UpdateContainerAndUpload(
                     &container,
                     ArtifactBlob{std::move(digest), blob, /*is_exec=*/false},
                     /*exception_is_fatal=*/true,