Skip to content

Commit

Permalink
Remove dead code in StorageManager and Query (#5193)
Browse files Browse the repository at this point in the history 
With the removal of deprecated function in #5146, the opportunity arises
to remove asynchronous query submission, which the C API no longer has
any reference to. Alongside this, there is other dead code in
`StorageManager` in need of pruning. This PR simplifies its constructor
a bit and removes unused members variables.

[SC-50038]

---
TYPE: NO_HISTORY
DESC: Remove dead code in `StorageManager` and `Query`
  • Loading branch information
@eric-hughes-tiledb
eric-hughes-tiledb authored Jul 18, 2024
1 parent 3275bd5 commit 2b377d8
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 151 deletions.
12 changes: 3 additions & 9 deletions tiledb/api/c_api_test_support/storage_manager_stub/storage_manager_override.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,32 +38,26 @@
#include "tiledb/sm/storage_manager/context_resources.h"

namespace tiledb::common {
class ThreadPool;
class Logger;
} // namespace tiledb::common
namespace tiledb::stats {
class Stats;
}
namespace tiledb::sm {
class Config;
class VFS;

class StorageManagerStub {
Config config_;

public:
static constexpr bool is_overriding_class = true;
StorageManagerStub(
ContextResources&, std::shared_ptr<common::Logger>, const Config& config)
ContextResources&,
const std::shared_ptr<common::Logger>&,
const Config& config)
: config_(config) {
}

inline Status cancel_all_tasks() {
return Status{};
}
inline Status set_tag(const std::string&, const std::string&) {
return Status{};
}
};

} // namespace tiledb::sm
Expand Down
18 changes: 0 additions & 18 deletions tiledb/sm/query/query.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1638,24 +1638,6 @@ Status Query::submit() {
return Status::Ok();
}

Status Query::submit_async(
std::function<void(void*)> callback, void* callback_data) {
// Do not resubmit completed reads.
if (type_ == QueryType::READ && status_ == QueryStatus::COMPLETED) {
callback(callback_data);
return Status::Ok();
}
init();
if (array_->is_remote())
return logger_->status(
Status_QueryError("Error in async query submission; async queries not "
"supported for remote arrays."));

callback_ = callback;
callback_data_ = callback_data;
return storage_manager_->query_submit_async(this);
}

QueryStatus Query::status() const {
return status_;
}
Expand Down
8 changes: 0 additions & 8 deletions tiledb/sm/query/query.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -705,14 +705,6 @@ class Query {
/** Submits the query to the storage manager. */
Status submit();

/**
* Submits the query to the storage manager. The query will be
* processed asynchronously (i.e., in a non-blocking manner).
* Once the query is completed, the input callback function will
* be executed using the input callback data.
*/
Status submit_async(std::function<void(void*)> callback, void* callback_data);

/** Returns the query status. */
QueryStatus status() const;

Expand Down
76 changes: 7 additions & 69 deletions tiledb/sm/storage_manager/storage_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,41 +32,10 @@

#include "tiledb/common/common.h"

#include <algorithm>
#include <functional>
#include <iostream>
#include <sstream>

#include "tiledb/common/heap_memory.h"
#include "tiledb/common/logger.h"
#include "tiledb/common/memory.h"
#include "tiledb/sm/array/array.h"
#include "tiledb/sm/array/array_directory.h"
#include "tiledb/sm/array_schema/array_schema.h"
#include "tiledb/sm/array_schema/array_schema_evolution.h"
#include "tiledb/sm/array_schema/array_schema_operations.h"
#include "tiledb/sm/array_schema/enumeration.h"
#include "tiledb/sm/consolidator/consolidator.h"
#include "tiledb/sm/enums/array_type.h"
#include "tiledb/sm/enums/layout.h"
#include "tiledb/sm/enums/object_type.h"
#include "tiledb/sm/filesystem/vfs.h"
#include "tiledb/sm/global_state/global_state.h"
#include "tiledb/sm/global_state/unit_test_config.h"
#include "tiledb/sm/group/group.h"
#include "tiledb/sm/misc/parallel_functions.h"
#include "tiledb/sm/misc/tdb_time.h"
#include "tiledb/sm/object/object.h"
#include "tiledb/sm/object/object_mutex.h"
#include "tiledb/sm/query/query.h"
#include "tiledb/sm/rest/rest_client.h"
#include "tiledb/sm/storage_manager/storage_manager.h"
#include "tiledb/sm/tile/generic_tile_io.h"
#include "tiledb/sm/tile/tile.h"

#include <algorithm>
#include <iostream>
#include <sstream>

using namespace tiledb::common;

Expand All @@ -83,29 +52,22 @@ class StorageManagerException : public StatusException {
/* CONSTRUCTORS & DESTRUCTORS */
/* ****************************** */

/*
* Note: The logger argument is no longer used, but has not been removed from
* the constructor signature as yet.
*/
StorageManagerCanonical::StorageManagerCanonical(
ContextResources& resources,
shared_ptr<Logger> logger,
const shared_ptr<Logger>&, // unused
const Config& config)
: resources_(resources)
, logger_(logger)
: vfs_(resources.vfs())
, cancellation_in_progress_(false)
, config_(config)
, queries_in_progress_(0) {
/*
* This is a transitional version the implementation of this constructor. To
* complete the transition, the `init` member function must disappear.
*/
throw_if_not_ok(init());
}

Status StorageManagerCanonical::init() {
auto& global_state = global_state::GlobalState::GetGlobalState();
global_state.init(config_);

global_state.register_storage_manager(this);

return Status::Ok();
}

StorageManagerCanonical::~StorageManagerCanonical() {
Expand All @@ -127,24 +89,6 @@ StorageManagerCanonical::~StorageManagerCanonical() {
/* API */
/* ****************************** */

Status StorageManagerCanonical::async_push_query(Query* query) {
cancelable_tasks_.execute(
&resources_.compute_tp(),
[this, query]() {
// Process query.
throw_if_not_ok(query_submit(query));
return Status::Ok();
},
[query]() {
// Task was cancelled. This is safe to perform in a separate thread,
// as we are guaranteed by the thread pool not to have entered
// query->process() yet.
throw_if_not_ok(query->cancel());
});

return Status::Ok();
}

Status StorageManagerCanonical::cancel_all_tasks() {
// Check if there is already a "cancellation" in progress.
bool handle_cancel = false;
Expand All @@ -159,8 +103,7 @@ Status StorageManagerCanonical::cancel_all_tasks() {
// Handle the cancellation.
if (handle_cancel) {
// Cancel any queued tasks.
cancelable_tasks_.cancel_all_tasks();
throw_if_not_ok(resources_.vfs().cancel_all_tasks());
throw_if_not_ok(vfs_.cancel_all_tasks());

// Wait for in-progress queries to finish.
wait_for_zero_in_progress();
Expand Down Expand Up @@ -198,11 +141,6 @@ Status StorageManagerCanonical::query_submit(Query* query) {
return st;
}

Status StorageManagerCanonical::query_submit_async(Query* query) {
// Push the query into the async queue
return async_push_query(query);
}

void StorageManagerCanonical::wait_for_zero_in_progress() {
std::unique_lock<std::mutex> lck(queries_in_progress_mtx_);
queries_in_progress_cv_.wait(
Expand Down
58 changes: 11 additions & 47 deletions tiledb/sm/storage_manager/storage_manager_canonical.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,11 @@ using namespace tiledb::common;

namespace tiledb::sm {

class Array;
class ArrayDirectory;
class ArraySchema;
class Consolidator;
class EncryptionKey;
class Query;

enum class EncryptionType : uint8_t;

/** The storage manager that manages pretty much everything in TileDB. */
/** The storage manager that manages pretty much nothing in TileDB. */
class StorageManagerCanonical {
public:
/* ********************************* */
Expand All @@ -80,24 +75,18 @@ class StorageManagerCanonical {
/**
* Complete, C.41-compliant constructor
*
* The `resources` argument is only used for its `vfs()` member function. This
* is the VFS instance that's waited on in `cancel_all_tasks`.
*
*
* @param resources Resource object from associated context
* @param config The configuration parameters.
*/
StorageManagerCanonical(
ContextResources& resources,
shared_ptr<Logger> logger,
const shared_ptr<Logger>& logger,
const Config& config);

private:
/**
* Initializes the storage manager. Only used in the constructor.
*
* TODO: Integrate what this function does into the constructor directly.
* TODO: Eliminate this function.
*
* @return Status
*/
Status init();

public:
/** Destructor. */
~StorageManagerCanonical();
Expand All @@ -109,14 +98,6 @@ class StorageManagerCanonical {
/* API */
/* ********************************* */

/**
* Pushes an async query to the queue.
*
* @param query The async query.
* @return Status
*/
Status async_push_query(Query* query);

/** Cancels all background tasks. */
Status cancel_all_tasks();

Expand All @@ -126,18 +107,6 @@ class StorageManagerCanonical {
/** Submits a query for (sync) execution. */
Status query_submit(Query* query);

/**
* Submits a query for async execution.
*
* @param query The query to submit.
* @return Status
*/
Status query_submit_async(Query* query);

[[nodiscard]] inline ContextResources& resources() const {
return resources_;
}

private:
/* ********************************* */
/* PRIVATE DATATYPES */
Expand Down Expand Up @@ -177,11 +146,10 @@ class StorageManagerCanonical {
/* PRIVATE ATTRIBUTES */
/* ********************************* */

/** Context create resources object. */
ContextResources& resources_;

/** The class logger. */
shared_ptr<Logger> logger_;
/**
* VFS instance used in `cancel_all_tasks`.
*/
VFS& vfs_;

/** Set to true when tasks are being cancelled. */
bool cancellation_in_progress_;
Expand All @@ -201,10 +169,6 @@ class StorageManagerCanonical {
/** Guards queries_in_progress_ counter. */
std::condition_variable queries_in_progress_cv_;

/** Tracks all scheduled tasks that can be safely cancelled before execution.
*/
CancelableTasks cancelable_tasks_;

/* ********************************* */
/* PRIVATE METHODS */
/* ********************************* */
Expand Down

0 comments on commit 2b377d8

Please sign in to comment.