Implement the SharePriceMovementPerMinute projection#43
Implement the SharePriceMovementPerMinute projection#43Artem-Semenov-dev wants to merge 60 commits intomasterfrom
SharePriceMovementPerMinute projection#43Conversation
… projection to the `point`.
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@Artem-Semenov-dev please see my comments. We are getting really close, but still I believe we should simplify internal things.
| } | ||
| } | ||
|
|
||
| private S onceUpdated() { |
There was a problem hiding this comment.
Let's document this method as well.
I think it is now called waitForUpdate().
| @@ -68,10 +97,29 @@ private void setState(S value) { | |||
|
|
|||
| private S state() { | |||
There was a problem hiding this comment.
I am not sure why this method still waits for something. What is the expected scenario? Maybe it should return @Nullable state instead?
I also don't understand why clearState() still exists, instead of instantiating future field each time when we need this "waiting" for the result.
I think, state() also deserves some documentation.
There was a problem hiding this comment.
I still believe that we need to clear the state of the entity before we send a command to the server. I think that the client observer on the entity state is triggered immediately (or after a very little amount of time) after we send a command, so if we gonna clear the state of the entity after sending a command, it will reinitialize the future which has been already triggered by the command.
There was a problem hiding this comment.
@Artem-Semenov-dev the thing here is that clearState() does not clear the state of the entity.
I would think that we should remember the last update of S value, and return it here. While all the gymnastics with futures should stay in setState and waitForUpdate.
Let's discuss this vocally.
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@Artem-Semenov-dev please see my comments.
| import io.spine.base.EntityState; | ||
| import io.spine.client.Client; | ||
| import io.spine.core.UserId; | ||
| import org.jetbrains.annotations.Nullable; |
There was a problem hiding this comment.
Please use a proper annotation.
It should look like this:
import org.checkerframework.checker.nullness.qual.Nullable;
...
public @Nullable TenantId tenantId() {
return tenantId;
}| @@ -68,10 +97,29 @@ private void setState(S value) { | |||
|
|
|||
| private S state() { | |||
There was a problem hiding this comment.
@Artem-Semenov-dev the thing here is that clearState() does not clear the state of the entity.
I would think that we should remember the last update of S value, and return it here. While all the gymnastics with futures should stay in setState and waitForUpdate.
Let's discuss this vocally.
| .getShareList(); | ||
| return shares; | ||
| var marketShares = availableMarketShares.state(); | ||
| Preconditions.checkNotNull(marketShares); |
There was a problem hiding this comment.
This should be imported statically.
And also, that should probably be requireNonNull, which is a part of JDK.
Same below.
| throw illegalStateWithCauseOf(e); | ||
| } | ||
| } | ||
| super(consumer -> client.onBehalfOf(user) |
There was a problem hiding this comment.
Is it possible to extract these lambda expressions to methods?
There was a problem hiding this comment.
It is not possible. We can access the methods before the supertype constructor call only if they are static, but we have a generic type S. So in our case, we cannot extract these lambdas to methods.
| /** | ||
| * Observer for entity state changes. | ||
| * | ||
| * <p>It allows to observe the asynchronous mutations of the entity state. |
| * <p>It allows to observe the asynchronous mutations of the entity state. | ||
| * | ||
| * @param <S> | ||
| * the state to observe. |
There was a problem hiding this comment.
In Java, we don't put periods at the end of parameter descriptions.
|
|
||
| private CompletableFuture<S> future = new CompletableFuture<>(); | ||
|
|
||
| private S state = null; |
There was a problem hiding this comment.
In which case, the field is @Nullable.
Same below.
| * and these states were introduced to restrict the order of the observing operations. | ||
| */ | ||
| private enum ObservationState { | ||
| OBSERVED, |
There was a problem hiding this comment.
This name is confusing. Can you please document each enum member, so that I could come up with alternative name?
Please think about this one too.
| future = new CompletableFuture<>(); | ||
| } | ||
| try { | ||
| S updatedEntity = future.whenComplete((value, error) -> { |
There was a problem hiding this comment.
Let's reduce code nesting.
Maybe, extract some bits?
| /** | ||
| * Represents the states of the entity observation. | ||
| * | ||
| * <p>The `AsyncObserver` can observe asynchronous changes of the entity's state, |
| import static java.time.Duration.ofSeconds; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| @DisplayName("'AsyncObserver' should") |
There was a problem hiding this comment.
We usually use back-ticks in display names. Yes, I know it looks funny along with my previous comment.
|
|
||
| public AsyncStateMutator(String state, Duration delay) { | ||
| this.state.set(state); | ||
| mutationNotifier = consumer -> |
…in the `E2EUser`.
…UPDATE_UNPACKED`.
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@Artem-Semenov-dev I appreciate the movement towards addressing previous concerns and comments. And it looks much better now. However, as discussed separately, we should do a bit more.
- Shorten the implementation of
EntitySubscription's ctor, as discussed via chat. - Get rid of
Consumer<Consumer<S>>by
a. definitely extracting, naming and documenting the inner part (Consumer<S>),
b. and most likely, extracting the outerConsumer<whatever the one above is going to be called>into something meaningful.
This is for sure. I will continue reviewing the rest of the changes, once these comments are addressed.
|
@Artem-Semenov-dev once again, I am reassigning this PR to myself now. Will pick it up once I get a chance. |
2 participants
This PR implements the
SharePriceMovementprojection, which allows us to observe the price movement of a particular share over a one-minute period. This projection will be used to draw the share price charts on the corresponding page of the application.