Skip to content

Conversation

@kinyoklion
Copy link
Member

No description provided.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #202753: Ability to persist and restore flags..

using launchdarkly::client_side::PersistenceBuilder;
using launchdarkly::config::shared::builders::LoggingBuilder;

class FilePersistence : public IPersistence {
Copy link
Member Author

Choose a reason for hiding this comment

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

Simple persistence implementation for demonstration purposes.

.Logging(LoggingBuilder::BasicLogging().Level(LogLevel::kDebug))
.Events(launchdarkly::client_side::EventsBuilder().FlushInterval(
std::chrono::seconds(5)))
.Persistence(
Copy link
Member Author

Choose a reason for hiding this comment

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

Configure persistence with the custom implementation.

.value(),
ContextBuilder().kind("user", "ryan").build());

auto before_init = client.BoolVariationDetail("my-boolean-flag", false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Read a flag before init is complete. Result can be different if the value was persisted.

}

DataSourceEventHandler::DataSourceEventHandler(
Context const& context,
Copy link
Member Author

Choose a reason for hiding this comment

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

Context flows through data source updates now. Allowing correlation between context and data. The dotnet client does this as well.

@@ -1,47 +1,36 @@
#include <tl/expected.hpp>
Copy link
Member Author

Choose a reason for hiding this comment

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

What was the FlagManager is now the FlagStore. The FlagManager coordinates the store, updater, and persistence.

@kinyoklion kinyoklion requested a review from cwaldren-ld May 18, 2023 16:38
@kinyoklion kinyoklion marked this pull request as ready for review May 18, 2023 16:39
char const** read_value,
void* user_data);

typedef void (*FreeFn)(char const* value, void* user_data);
Copy link
Member Author

Choose a reason for hiding this comment

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

So, when we read a value someone has to allocate that value. I figured I let the implementer allocate it. Which means that the implementer needs to know when to free it.

Alternatively I could do some buffer scheme, where I see if the read fits, grow the buffer, or do partial reads. This seemed simpler.

}

ContextIndex FlagPersistence::GetIndex() {
if (persistence_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could early out.

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 doing the other one, but this one it makes it so multiple different paths need to create a ContextIndex, so the nesting is more consice.

Logger& logger_;
std::size_t max_cached_contexts_;

IDataSourceUpdateSink* sink_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we passing the sink as a raw pointer again - What's the ownership model?

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 is 1 in the client and it is passed around. Currently it isn't consistently used. I think a pointer is better than a reference for this use case.

But, if we change either use case, we should probably just make them all shared pointers.

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 can make it a reference for now.

@kinyoklion kinyoklion requested a review from cwaldren-ld May 20, 2023 00:09
@kinyoklion kinyoklion merged commit c50e0d1 into main May 22, 2023
@kinyoklion kinyoklion deleted the rlamb/sc-202753/persistence-interface branch May 22, 2023 18:29
@github-actions github-actions bot mentioned this pull request May 22, 2023
@github-actions github-actions bot mentioned this pull request May 13, 2024
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