Skip to content

Conversation

@kinyoklion
Copy link
Member

No description provided.


Client client(ConfigBuilder(key).Build().value(),
ContextBuilder().kind("user", "ryan").build());
Client client(
Copy link
Member Author

Choose a reason for hiding this comment

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

Made this use polling for now. So I could watch it poll.

* will be used.
*/
class StreamingDataHandler {
class DataSourceEventHandler {
Copy link
Member Author

Choose a reason for hiding this comment

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

Used by both polling and streaming.


using launchdarkly::client_side::data_sources::DataSourceStatus;

static std::unique_ptr<IDataSource> MakeDataSource(
Copy link
Member Author

Choose a reason for hiding this comment

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

Once we have file and test data sources we may need to scale this differently.

std::lock_guard lock(status_mutex_);

// If initializing, then interruptions remain initializing.
auto new_state =
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 missed this bit of logic before.

<< (retry_message ? *retry_message : "giving up permanently");
}

bool IsInvalidSdkKeyStatus(HttpResult::StatusCode code) {
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 should maybe find a different home for this?

@kinyoklion kinyoklion requested a review from cwaldren-ld April 25, 2023 22:42
@kinyoklion kinyoklion marked this pull request as ready for review April 25, 2023 22:43
message, std::chrono::system_clock::now());
}

data_source_status_signal_(std::move(Status()));
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

encountering the same in my event processor stuff. I figure, I'll accept a std::function and then the caller can dispatch it to some strand using asio::post

// Sit around.
std::string t;
std::cout << "Press enter to exit" << std::endl;
std::cin >> t;
Copy link
Member Author

Choose a reason for hiding this comment

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

There may be some problem actually closing. So I need to look at that.

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 left it overnight and then it just didn't exit.

Context const& context,
config::detail::built::ServiceEndpoints const& endpoints,
config::detail::built::HttpProperties const& http_properties,
std::chrono::seconds polling_interval,
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been kind of debating this on my end, but wondering if we should just pass in the Config to keep the number of parameters down.

After all, it (should) be very easy to build a fully valid config with just a small delta for your use/test case.

That'd eliminate the sdk_key, endpoints, http_properties, and polling_interval

Copy link
Member Author

Choose a reason for hiding this comment

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

Streaming and polling config have been re-worked with this and the defaults.

inline const static std::string polling_get_path_ = "/msdk/evalx/contexts";

inline const static std::string polling_report_path_ =
"/msdk/evalx/context";
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well pull these into the PollingConfig?

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 still don't feel like these are config. You can never change them correct? Maybe I just need to make a place for constants.

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 guess if I think of the possibility of sharing the implementation, then possibly they could be configurable. In the sense that they may differ between implementations.

Copy link
Contributor

@cwaldren-ld cwaldren-ld Apr 26, 2023

Choose a reason for hiding this comment

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

I believe they are config even if they aren't user-facing, since they are config for us. I think it's nice to be able to see all of those variables in one place, like in Defaults (plus makes testing easier)

message, std::chrono::system_clock::now());
}

data_source_status_signal_(std::move(Status()));
Copy link
Contributor

Choose a reason for hiding this comment

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

encountering the same in my event processor stuff. I figure, I'll accept a std::function and then the caller can dispatch it to some strand using asio::post


namespace launchdarkly::client_side::data_sources::detail {

const static std::chrono::seconds kMinPollingInterval =
Copy link
Contributor

Choose a reason for hiding this comment

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

Could pull into PollingConfig.

// and it didn't have an etag, we still don't want to try to
// parse the body.
} else {
if (network::detail::IsRecoverableStatus(res.Status())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I wrote one of these too. We can compare once you merge this PR, maybe I can delete mine..

@cwaldren-ld cwaldren-ld self-requested a review April 26, 2023 19:56
@kinyoklion kinyoklion merged commit 7ef503b into main Apr 26, 2023
@kinyoklion kinyoklion deleted the rlamb/implement-polling-2 branch April 26, 2023 20:16
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.

4 participants