DataStorm fixes for configuration, session retries, and more #3166
Conversation
| <property name="Topic.DiscardPolicy" default="Never" languages="cpp" /> | ||
| <property name="Topic.Priority" default="0" languages="cpp" /> | ||
| <property name="Topic.SampleCount" default="-1" languages="cpp" /> | ||
| <property name="Topic.SampleLifetime" default="0" languages="cpp" /> |
There was a problem hiding this comment.
Topic properties where not added before, but they are public and documented in
https://doc.zeroc.com/datastorm/latest/property-reference/datastorm-topic
I updated the docs to include -1 default for SampleCount this was missing in the docs.
| @@ -517,7 +517,6 @@ SessionI::connected(SessionPrx session, const Ice::ConnectionPtr& connection, co | |||
| lock_guard<mutex> lock(_mutex); | |||
| if (_destroyed || _session) | |||
| { | |||
| assert(_connectedCallbacks.empty()); | |||
There was a problem hiding this comment.
Removed unused _connectedCallbacks we never add any callbacks.
| for (auto& t : _topics) | ||
| { | ||
| runWithTopics(t.first, [&](TopicI* topic, TopicSubscriber&) { topic->detach(t.first, shared_from_this()); }); | ||
| runWithTopics(t.first, [id = t.first, self](TopicI* topic, TopicSubscriber&) { topic->detach(id, self); }); |
There was a problem hiding this comment.
minor change, no need to call shared_from_this in the loop or capture everything by ref.
There was a problem hiding this comment.
Could you declare self in the capture block to avoid an extra copy?
| @@ -655,20 +650,17 @@ SessionI::retry(NodePrx node, exception_ptr exception) | |||
| } | |||
| } | |||
|
|
|||
| if (node->ice_getEndpoints().empty() && node->ice_getAdapterId().empty()) | |||
| // Cancel any pending retry task, before we start a new one below. | |||
| if (_retryTask) | |||
There was a problem hiding this comment.
Previous code only canceled the retry task when there is no endpoints. Here we are about to schedule a new task I think is clear to cancel any pending task.
| @@ -678,16 +670,15 @@ SessionI::retry(NodePrx node, exception_ptr exception) | |||
| << " (ms) for peer to reconnect"; | |||
| } | |||
|
|
|||
| // Schedule a timer to remove the session if the peer doesn't reconnect. | |||
| _retryTask = make_shared<IceInternal::InlineTimerTask>([self = shared_from_this()] { self->remove(); }); | |||
There was a problem hiding this comment.
Here SessionI::remove would call retry again, and we schedule a new call to remove.
As far as I see we have and endless loop of calling retry to print this message and scheduling the remove call that just calls retry.
| if (exception) | ||
| { | ||
| try | ||
| { | ||
| rethrow_exception(exception); | ||
| } | ||
| catch (const Ice::LocalException& ex) | ||
| catch (const std::exception& ex) |
There was a problem hiding this comment.
This is to match what we do above, when we handle what exceptions to retry.
| is >> value; | ||
| return value; | ||
| } | ||
|
|
There was a problem hiding this comment.
unused after updating handling of configurations.
| if (value == "Never") | ||
| { | ||
| return DataStorm::DiscardPolicy::None; | ||
| } |
There was a problem hiding this comment.
We have this discrepancy, documented value is "Never", but the enum value is None.
There was a problem hiding this comment.
I assume Never is the "right" choice here.
There was a problem hiding this comment.
DiscardPolicy is part of the public API. User code can still set it manually using DiscardPolicy::None.
Should we rename DiscardPolicy::None to DiscardPolicy::Never?
There was a problem hiding this comment.
Or parse both "Never" and "None" to DiscardPolicy::None?
There was a problem hiding this comment.
I think it's preferable to just have one value.
| { | ||
| auto p = _listeners.find(ListenerKey{session}); |
There was a problem hiding this comment.
ListenerKey was not required for this map, we were using the default shared_ptr operator<
| @@ -876,8 +868,7 @@ TopicReaderI::TopicReaderI( | |||
| std::move(name), | |||
| id) | |||
| { | |||
| _defaultConfig = {-1, 0, DataStorm::ClearHistoryPolicy::OnAll, DataStorm::DiscardPolicy::None}; | |||
There was a problem hiding this comment.
Move the defaults to the property defaults, and update the code to use getIceProperty.
| else if (p->second == "SendTime") | ||
| { | ||
| config.discardPolicy = DataStorm::DiscardPolicy::Priority; | ||
| } |
There was a problem hiding this comment.
This was bogus, but it seems we don't have any tests that catches this error.
| for (auto& t : _topics) | ||
| { | ||
| runWithTopics(t.first, [&](TopicI* topic, TopicSubscriber&) { topic->detach(t.first, shared_from_this()); }); | ||
| runWithTopics(t.first, [id = t.first, self](TopicI* topic, TopicSubscriber&) { topic->detach(id, self); }); |
There was a problem hiding this comment.
Could you declare self in the capture block to avoid an extra copy?
| if (value == "Never") | ||
| { | ||
| return DataStorm::DiscardPolicy::None; | ||
| } |
There was a problem hiding this comment.
I assume Never is the "right" choice here.
| if (value == "Never") | ||
| { | ||
| return DataStorm::DiscardPolicy::None; | ||
| } |
There was a problem hiding this comment.
Or parse both "Never" and "None" to DiscardPolicy::None?
…ssion retries, and more (zeroc-ice/ice#3166)
…ssion retries, and more (zeroc-ice/ice#3166)
No description provided.