-
Notifications
You must be signed in to change notification settings - Fork 11
chore: Add FDv2 data source interfaces. #100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| */ | ||
| public static class Status { | ||
| private final State state; | ||
| private final DataSourceStatusProvider.ErrorInfo errorInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly care for this type, but also we may not want what is effectively a duplicate.
lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/datasources/FDv2SourceResult.java
Outdated
Show resolved
Hide resolved
| * an FDv2 synchronizer produces a stream of results. | ||
| */ | ||
| public class FDv2SourceResult { | ||
| public enum State { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want running in this enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I shouldn't call it State. Its more like a notification of change, and it doesn't represents valid/running because that is handled by a changeset.
| /** | ||
| * The data source has been instructed to disconnect and will not produce any further results. | ||
| */ | ||
| GOODBYE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current description of GOODBYE and SHUTDOWN leaves room for me to think GOODBYE -> SHUTDOWN could be a thing.
I see the diagrams in the specific ones down below that shows that is not possible, but the enum details make it look like it could be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GOODBYE is effectively a more specific shutdown. We didn't handle it in .Net because SDKs generally don't seem to have handled it yet, but it also is really hard to handle with data source updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically in .Net we had an overload broad OFF and it is hard to get the correct behavior because there were are really 2-3 different meanings. Overall we don't need to emit any data source status in most situations for something being shutdown or something being told to exit by flag delivery (but we may handle them different, I am still unsure how to handle goodbye).
SHUTDOWN means I asked the thing to shutdown.
GOODBYE means that FD or some external source asked it to disconnect. Having it different means we can decide to do something different. (I think just by entering this situation we handle most of the problems, because we can be like, cool we aren't running anymore, I don't care about this disconnect I just got from the event source.)
TERMINAL_ERROR means something has gone wrong.
dcbd9fd to
4b8313b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very early scaffold that gives an idea how this stuff would connect together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convenience class that allows for easy mapping of producer/consumer to an iterator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encapsulates all the poll-specific logic which is then used by the polling synchronizer/initializer.
…unchdarkly/java-core into rlamb/add-fdv2-data-source-interfaces
tanderson-ld
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting partial review, will resume tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some usage of "poll / polling" in this file, not sure if you want to eliminate that so if this requestor is used elsewhere, the concept of polling doesn't come with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change the types in the interface level, but this implementation is specific to our polling implementation and the format of data it provides as well as the cycle it operates on.
|
|
||
| if (selector.getVersion() > 0) { | ||
| requestUri = HttpHelpers.addQueryParam(requestUri, VERSION_QUERY_PARAM, String.valueOf(selector.getVersion())); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this version constraint on the Dotnet impl. I'm wondering why this ever was imposed? What if 0 is valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a couple reasons. 0 and not present would be the same, and 0 is the "zero" value for the go implementation.
That said this should use !selector.isEmpty() and I will change it.
|
|
||
| // Parse the response body | ||
| if (response.body() == null) { | ||
| future.completeExceptionally(new IOException("Response body is null")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a null body necessarily an IO Exception? Seems like an application layer exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It represents an implementation error at this level. Though it isn't strictly an IOError, but it is better than a segfault/NPE. (Which seems to be what the existing implementation would do if this was somehow possible.)
There are a series of conditions which can make this null, which the layer above this shouldn't know about.
It could be an empty string, at which point the attempt at parsing would resolve the error, but that would be different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a comment.
| * This synchronizer is recovering from a previous failure and will be available to use | ||
| * after a delay. | ||
| */ | ||
| Recovering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe TempBlocked or OnCooldown? Recovering seems to imply we know it is doing something to recover, but we don't know anything about its internals to glean that. If you like TempBlocked, consider Blocked -> PermBlocked.
lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/FDv2DataSource.java
Outdated
Show resolved
Hide resolved
lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/FDv2DataSource.java
Show resolved
Hide resolved
| // TODO: Log. | ||
| // Move to next synchronizer. | ||
| } | ||
| availableSynchronizer = getFirstAvailableSynchronizer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this deviates from the spec slightly, but it is probably ok, and possibly an improvement?
| @Override | ||
| public boolean isInitialized() { | ||
| try { | ||
| return startFuture.isDone() && startFuture.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok that startFuture.get() could block? I think existing data sources interacted with atomic booleans for the return.
| @Override | ||
| public void close() throws IOException { | ||
| // If this is already set, then this has no impact. | ||
| startFuture.complete(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to interrupt initializers?
BEGIN_COMMIT_OVERRIDE
chore: Add FDv2 data source interfaces.
chore: Add FDv2 polling data source.
chore: Scaffold FDv2 Data Source.
END_COMMIT_OVERRIDE