Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harden against object lifetime on exceptions #1928

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

willdealtry
Copy link
Collaborator

Throw from pipeline and storages to check resiliency

@willdealtry willdealtry changed the title Chaos testing do not merge Harden against object lifetime on exceptions Oct 16, 2024
@willdealtry willdealtry marked this pull request as ready for review October 17, 2024 08:31
@@ -954,7 +954,7 @@ if(${TEST})
python/python_handlers.cpp
storage/test/common.hpp
version/test/test_sort_index.cpp
)
util/test/random_throw.hpp)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: alphabetical order

@@ -507,6 +510,7 @@ struct MemSegmentFunctionTask : BaseTask {
ARCTICDB_MOVE_ONLY_DEFAULT(MemSegmentFunctionTask)

folly::Unit operator()(std::pair<VariantKey, SegmentInMemory> &&seg_pair) {
ARCTICDB_DEBUG_THROW(5)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This task is unused, assume this should be in MemSegmentProcessingTask

@@ -100,6 +100,7 @@ std::vector<EntityId> PassthroughClause::process(std::vector<EntityId>&& entity_
}

std::vector<EntityId> FilterClause::process(std::vector<EntityId>&& entity_ids) const {
ARCTICDB_DEBUG_THROW(5)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just stick this in the base class to avoid the duplication
https://github.com/man-group/ArcticDB/blob/master/cpp/arcticdb/processing/clause.hpp#L59-L61
And could put one in the structure_for_processing overload that takes entity_ids_vec as well for good measure.

@@ -1435,10 +1435,10 @@ VersionedItem collate_and_write(
});
}

void delete_incomplete_keys(PipelineContext* pipeline_context, Store* store) {
void delete_incomplete_keys(PipelineContext& pipeline_context, Store& store) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing PipelineContext can be const&

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A rational person would think so, but because of irritating reasons the function returning the iterator into the pipeline context can't be made easily const, and we want this class to just go away so I don't want to spend too much time (any time) polishing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants