shared storage limit #2222
shared storage limit #2222
Conversation
MBoldyrev
force-pushed
the
wip/mst-shared-limit
branch
from
9b98ea7
to
a3e6f62
Compare
| logger::LoggerPtr log) | ||
| : log_(std::move(log)), | ||
| pcs_(pcs), | ||
| pending_batches_(storage_limit->create<InternalStorage>( |
There was a problem hiding this comment.
I guess better to create storage in the outer scope of the ctor and pass it here.
| // when state does not contain transaction | ||
| if (this->rawInsert(rhs_batch)) { | ||
| // there is enough room for the new batch | ||
| BOOST_VERIFY_MSG(state_update.updated_state_->rawInsert(rhs_batch), |
There was a problem hiding this comment.
What is the point to check the insertion twice?
There was a problem hiding this comment.
here we check the insertion to state update, which is expected to accept the batch unconditionally (it has no limits). and above we check the insertion to local storage, which is limited and may deny new batch, which is OK.
| log_->info("Dropped a batch because it did not fit into storage: {}", | ||
| *rhs_batch); | ||
| } | ||
| return {}; |
There was a problem hiding this comment.
The method should return void as I see. What is the default value is assumed here?
There was a problem hiding this comment.
it is a lambda that extracts batches to be moved from MstState's internal storage. The return type is any iterable, in this case it is a vector: https://github.com/hyperledger/iroha/blob/a3e6f62460abc4c086a94ca32c4c0c19ca7b98a8/irohad/multi_sig_transactions/state/impl/mst_state.cpp#L206
|
|
||
| namespace iroha { | ||
|
|
||
| struct LimitableStorage { |
There was a problem hiding this comment.
Add documentation for this and further classes/methods in this file.
There was a problem hiding this comment.
It looks like with this PR we want to solve two tasks:
- Limit some storage with an upper bound size
- Make "defer" remove from the collection.
The first task can be solved by StorageLimit strategy and this strategy should be passed into a collection as the dependency.
The second task should be solved by inheritance at the collection from an interface with such semantics:
template<typename Item>
class DeferRemove {
Proxy deferRemove(Key); // Key is some tag for removing objects from the collection here it is doesn't matter finalized interface. Only proxy is important here.
}
template<typename Item>
class Proxy {
Item release(); // method removes item from initial collection
private:
Iter<Item> obj; // there could be a pointer, iterator or identifier in the collection.
}
With this approach, all synchronization stuff will hold in the collection and will be consistent with the rest of the collection class and code will look cleaner because we separate two responsibilities - there is no intersection between add functionality in the collection and add which bad case of removing. Also, there is no need util classes such as MstToPcsPropagation because all stuff is doing in one subscribe on PCS.
Signed-off-by: Mikhail Boldyrev <[email protected]>
MBoldyrev
force-pushed
the
wip/mst-shared-limit
branch
from
a3e6f62
to
955cd2c
Compare
|
The PR got too big and could be split to two: this one brings a more general shared storage limit, and #2230 employs it to limit MST storages. |
Signed-off-by: Mikhail Boldyrev <[email protected]>
Description of the Change
A set of classes providing a shared limit between several storages.
Benefits
Possible Drawbacks
Usage Examples or Tests [optional]
please refer to
storage_shared_limit_test: https://github.com/MBoldyrev/iroha/blob/wip/mst-shared-limit/test/module/libs/storage_shared_limit/storage_shared_limit_test.cppAlternate Designs [optional]