DataStorm comments & minor fixes #3212
Conversation
| return DataSample{ | ||
| .id = sample->id, | ||
| .keyId = marshalKey ? 0 : sample->key->getId(), | ||
| .keyValue = marshalKey ? sample->key->encode(communicator) : Ice::ByteSeq{}, | ||
| .timestamp = chrono::time_point_cast<chrono::microseconds>(sample->timestamp).time_since_epoch().count(), | ||
| .tag = sample->tag ? sample->tag->getId() : 0, | ||
| .event = sample->event, | ||
| .value = sample->encode(communicator)}; |
There was a problem hiding this comment.
For large structs with many fields this style, using the filed names, seems easier to read.
| .lastIds = std::move(lastIds), | ||
| .samples = std::move(samples.samples), |
There was a problem hiding this comment.
We can move them, don't need to copy these containers.
| @@ -178,7 +189,7 @@ DataElementI::attach( | |||
| session->subscriberInitialized(topicId, id > 0 ? data.id : -data.id, data.samples, key, shared_from_this()); | |||
| if (!samplesI.empty()) | |||
| { | |||
| return [=, self = shared_from_this()]() | |||
| return [=, samplesI = std::move(samplesI), self = shared_from_this()]() | |||
There was a problem hiding this comment.
My understanding is that the previous code, captures samplesI as a copy, which is them passed by reference to initSamples.
I updated it to move capture it because it was not used anywhere else.
| @@ -634,7 +648,7 @@ DataElementI::forward(const Ice::ByteSeq& inParams, const Ice::Current& current) | |||
| { | |||
| for (const auto& [_, listener] : _listeners) | |||
| { | |||
| // If there's at least one subscriber interested in the update | |||
| // If we are forwarding a sample check if at least once of the listeners is interested in the sample. | |||
| if (!_sample || listener.matchOne(_sample, false)) | |||
There was a problem hiding this comment.
The !_sample check is there because this also forwards other operations. _sample is only set for SubscriberSession::s when sending a sample.
| @@ -216,7 +218,7 @@ SessionI::attachTags(int64_t topicId, ElementInfoSeq tags, bool initialize, cons | |||
|
|
|||
| runWithTopics( | |||
There was a problem hiding this comment.
We have two overloads of runWithTopics which are almost identical, only differing in the passed callback, one has an extra TopicSubscribers parameter that was never used. I got read of that overload and updated the callbacks to use the other one.
| fn(topic); | ||
| _topicLock = nullptr; | ||
| l.unlock(); |
There was a problem hiding this comment.
No need for the explicit unlock here, the lock is being destroyed as it is about to go out of scope.
| @@ -1222,15 +1202,16 @@ SubscriberSessionI::getTopics(const string& name) const | |||
| } | |||
|
|
|||
| void | |||
| SubscriberSessionI::s(int64_t topicId, int64_t elementId, DataSample s, const Ice::Current& current) | |||
| SubscriberSessionI::s(int64_t topicId, int64_t elementId, DataSample dataSample, const Ice::Current& current) | |||
| { | |||
There was a problem hiding this comment.
I kept the same logic, just renamed parameters, and loop variables to make things more readable.
| @@ -131,15 +131,31 @@ module DataStormContract | |||
| Ice::ByteSeq criteria; | |||
| } | |||
|
|
|||
| /// Represents the configuration of a reader or writer. | |||
| class ElementConfig(1) | |||
| { | |||
| optional(1) string facet; | |||
Co-authored-by: Joe George <[email protected]>
Co-authored-by: Joe George <[email protected]>
This PR includes additional fixes for DataStorm. Mostly doc comments and style fixes to make the code easier to follow.