Lt 5012 eod borders #516
Lt 5012 eod borders #516
Conversation
gponomarev-lykke
force-pushed
the
LT-5012-eod-borders
branch
from
2e46406
to
2250b1f
Compare
|
|
||
| namespace MarginTrading.Backend.Core.Services | ||
| { | ||
| public interface ISnapshotMonitor |
There was a problem hiding this comment.
The goal of this interface is suspicious.
3 methods change the state of snapshot and the last one checks if snapshot creation should be retried. I think too much responsibilities here for a single interface.
| /// If snapshot is not created after a specified amount of time, SnapshotMonitorService will retry this operation | ||
| /// </summary> | ||
|
|
||
| public TimeSpan SnapshotCreationTimeout { get; set; } = TimeSpan.FromMinutes(5); |
There was a problem hiding this comment.
Reading the comment, I have a feeling this is retry timeout. Shouldn't you call it then CreationRetryTimeout or something?
| public TimeSpan MonitoringDelay { get; set; } = TimeSpan.FromSeconds(30); | ||
|
|
||
| /// <summary> | ||
| /// If snapshot is not created after a specified amount of time, SnapshotMonitorService will retry this operation |
There was a problem hiding this comment.
either
| /// If snapshot is not created after a specified amount of time, SnapshotMonitorService will retry this operation | |
| /// If snapshot is not created after a specified amount of time, creation will be retried |
or
| /// If snapshot is not created after a specified amount of time, SnapshotMonitorService will retry this operation | |
| /// If snapshot is not created after a specified amount of time, <see cref="SnapshotMonitorService"/> will retry this operation |
src/MarginTrading.Backend.Services/AssetPairs/ScheduleSettingsCacheService.cs
Outdated
Show resolved
Hide resolved
src/MarginTrading.Backend.Services/Infrastructure/SnapshotMonitor.cs
Outdated
Show resolved
Hide resolved
| public void SnapshotFinished() | ||
| { | ||
| _state = DraftSnapshotState.None; | ||
| _timestamp = default; |
There was a problem hiding this comment.
Obviously, you expect monitor to be reused. In this case, probably, there should be a guard against not proper usage of it. For example, currently, I can call SnapshotRequested and then SnapshotFinished. Which seems not right.
There was a problem hiding this comment.
Good point, I'll think about it
|
|
||
| namespace MarginTrading.Backend.Services | ||
| { | ||
| public class SnapshotMonitorService : BackgroundService |
There was a problem hiding this comment.
I can’t spot even semantically the difference between SnapshotMonitor and SnapshotMonitorService but obviously they are different classes.
There was a problem hiding this comment.
SnapshotMonitorService cannot be injected into other services, as it implements IHostedService and is registered by AddHostedService.
SnapshotMonitor is responsible for tracking snapshot's state, and determines if timeout has expired and we need to retry the snapshot's creation process.
SnapshotMonitorService is a hosted service that is responsible for repeatedly checking the state of the SnapshotMonitor instance, and actually calling the snapshot's creation method (implemented in ISnapshotService)
There was a problem hiding this comment.
Your comment explains a lot, however it is not clear from the naming. Naming is similar and confuses. Next time someone reads the code, her or she will probably not have your comments unless specifically requested.
| try | ||
| { | ||
| await _snapshotService.MakeTradingDataSnapshot(command.TradingDay, command.OperationId); | ||
| var quotes = await _quotesApi.GetCfdQuotes(command.TradingDay); |
There was a problem hiding this comment.
As I see this creates an ugly dependency on BookKeeper which becomes circular dependency cause bookkeeper already consumes trading core APIs.
There was a problem hiding this comment.
Kinda, and it creates some startup issues as described in the comment about IsAlive. But since BookKeeper handles the quotes and actually stores them in its database, I think it's responsible for them and must be a single source of truth.
Previous implementations managed this in two ways:
- "missed" eod method simply pushed quotes into mt core api. This breaks the
CreateSnapshotCommandhandler, because there's no way to get quotes (we need to save them into a table inside mtcore, which leads to duplication) - "normal" eod would send quotes one-by-one via rabbitmq. This breaks the "missed" eod flow, because quotes would update internal caches of MtCore, and we should not send these outdated quotes and replace "normal" ones
My approach unifies both flows:
- BookKeeper is responsible for quotes.
- The
CreateSnapshotCommandhandler receives quotes from bookkeeper's IQuotesApi for both missed and normal flows, based only on TradingDay - During normal eod flow quotes are still being sent one-by-one and MtCore's caches are updated properly
- During missed eod flow quotes are not being sent one-by-one, and MtCore's caches are not being broken by them
There was a problem hiding this comment.
Do you have an estimate how much space the EOD quotes consume? Is it feasible/make sense to send them using message/command?
There was a problem hiding this comment.
It heavily depends on load. The object itself is not that big:
"Id": "BUGS2402",
"Timestamp": "2024-03-21T21:29:26.6565081Z",
"Bid": 112.0,
"Ask": 112.0
},
But if scaled to thousands of assets, it might become a bottleneck. I was thinking that I could add this data to CreateSnapshotCommand, but in the end decided against it.
The api in bookkeeper, on the other hand, can be refactored to return paginated response, and that is more scalable.
I think we can ship the current solution to the bank, and discuss it internally with Andrey A and Daniel when you return from vacation. I understand that it's controversial, and together we can come up with a better idea.
|
No description provided.