Skip to content

Conversation

@kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Jul 13, 2023

No description provided.

kinyoklion added 30 commits July 6, 2023 14:11
…arkly/cpp-sdks-private into rlamb/basic-store-implementation
@kinyoklion kinyoklion changed the base branch from main to server-side July 13, 2023 23:46
@kinyoklion kinyoklion changed the title Rlamb/implement server streaming data source feat: Implement streaming data source. Jul 13, 2023
}
return DataSourceEventHandler::Patch{
TStreamingDataKind::Key(path),
data_model::ItemDescriptor<TData>(data->value())};
Copy link
Member Author

Choose a reason for hiding this comment

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

I wish this lint worked, because it would be useful, but it is basically always wrong. Checked on line 41.

@kinyoklion kinyoklion force-pushed the rlamb/implement-server-streaming-data-source branch from 2ca520e to 77de4fd Compare July 14, 2023 17:20
return;
}

// TODO: Initial reconnect delay. sc-204393
Copy link
Member Author

Choose a reason for hiding this comment

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

Likely this should still be addressed as an independent change, but maybe this ticket should be pulled into the epic.
I think that the SSE client currently has a hardcoded value and it just needs to expose a value for building instead, and then we can use that value.

if (auto self = weak_self.lock()) {
self->data_source_handler_.HandleMessage(event.type(),
event.data());
// TODO: Use the result of handle message to restart the
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how to handle this in the current pattern. I think we went with re-creating instead of restarting. So it will require extracting out the build of the client, then closing and re-creating the client.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thought is have the callback return a bool/status that tells the eventsource to reconnect. Then we don't have to rebuild it and can keep the async stuff hidden there.

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds more pleasant.

@kinyoklion kinyoklion marked this pull request as ready for review July 14, 2023 17:58
@kinyoklion kinyoklion requested a review from a team July 14, 2023 17:58
PARSE_FIELD(path, obj, "path");

if (StreamingDataKinds::Flag::IsKind(path)) {
return Patch<StreamingDataKinds::Flag, data_model::Flag>(path, obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very elegant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I spent an inordinate amount of time on template shenanigans. Ultimately it is pretty pleasant though,

LD_LOG(logger_, LogLevel::kWarn)
<< "Polling for feature flag updates failed: "
<< (error_message.has_value() ? *error_message : "unknown error");
} else if (res.Status() == 200) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda wish we could use the beast error codes here. Maybe we can, they might just be the numeric values of the status codes or have a getter for it.

if (auto self = weak_self.lock()) {
self->data_source_handler_.HandleMessage(event.type(),
event.data());
// TODO: Use the result of handle message to restart the
Copy link
Contributor

Choose a reason for hiding this comment

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

Another thought is have the callback return a bool/status that tells the eventsource to reconnect. Then we don't have to rebuild it and can keep the async stuff hidden there.

kinyoklion and others added 7 commits July 14, 2023 14:55
…atus_manager_base.hpp

Co-authored-by: Casey Waldren <cwaldren@launchdarkly.com>
Co-authored-by: Casey Waldren <cwaldren@launchdarkly.com>
Co-authored-by: Casey Waldren <cwaldren@launchdarkly.com>
Co-authored-by: Casey Waldren <cwaldren@launchdarkly.com>
@kinyoklion kinyoklion requested a review from cwaldren-ld July 14, 2023 22:59

#include <launchdarkly/encoding/base_64.hpp>
#include <launchdarkly/serialization/json_flag.hpp>
// #include <launchdarkly/serialization/json_primitives.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@kinyoklion kinyoklion merged commit 08aaa66 into server-side Jul 17, 2023
@kinyoklion kinyoklion deleted the rlamb/implement-server-streaming-data-source branch July 17, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants