Multi Triggers -> Develop #395
Conversation
- TODO: Call IsEventFirstTime from the new db/localDataStore class
- Renamed one letter object names to fit the context - TODO: Call appropriate persist method from the new db/localDataStore class
- Added new class CTEventDatabase to store user events in database. - Added create table, insert, update and check event methods.
…ofile-events [MC-2088] Evaluate first time profile events
…ents [MC-2086] Evaluate first time events
- synchronised userEventLogs set access
[MC - 2090] In-memory storage events
[MC-2106] Add event storage limits
[MC-2082] Implement new event storage
[MC-2430] Normalize event names
…e to the database. updated tests. added normalised names to queries.
[MC-2429] Add new public apis - Multi triggers
Complete Todos and connect database code
There was a problem hiding this comment.
In - (void)processEvent:(NSDictionary *)event withType:(CleverTapEventType)eventType the event is written to the local storage and further down in the same method the evaluation manager is called to evaluate the in-apps for the event.
The local data store calls persistEvent which calls self.dbHelper upsertEvent in a background queue with dispatch_async. The database then runs the query in the database queue.
The evaluation is done on the serial queue on which the events are processed and queued.
The current flow is persistEvent, upsertEvent, evaluateOnEvent, eventExists, update success, upsert success. However, this does not seem guaranteed since code runs on different queues. This opens for potential race conditions. Please check this.
Please check the comments left. I have not finished reviewing all files but I believe the feedback provided is enough for discussion. I will leave additional comments if I have more suggestions.
| dispatch_queue_t _databaseQueue; | ||
| } | ||
|
|
||
| + (instancetype)sharedInstanceWithConfig:(CleverTapInstanceConfig *)config { |
There was a problem hiding this comment.
The CTEventDatabase shared instance with config is bit misleading that it can return a different instance based on the config and use a shared instance with same configs. Do we want separate databases per account id?
The CTEventDatabase is only used in the local store, does it need to be a singleton?
| - (instancetype)initWithConfig:(CleverTapInstanceConfig *)config { | ||
| if (self = [super init]) { | ||
| _config = config; | ||
| _databaseQueue = dispatch_queue_create([[NSString stringWithFormat:@"com.clevertap.eventDatabaseQueue:%@", _config.accountId] UTF8String], DISPATCH_QUEUE_CONCURRENT); |
There was a problem hiding this comment.
Using a concurrent queue for database access is generally discouraged unless reads and writes are separated or synchronized.
Also, all calls to this queue are dispatch_sync which negates its concurrency to be DISPATCH_QUEUE_CONCURRENT.
There was a problem hiding this comment.
I believe it is better to use the CTClock protocol (CTSystemClock implementation) to get the current timestamp (see the CTImpressionManager). This will allow easier mocking and testing.
| deviceID:kDeviceID]; | ||
|
|
||
| NSInteger firstTs = [self.eventDatabase getFirstTimestamp:self.normalizedEventName deviceID:kDeviceID]; | ||
| XCTAssertEqual(firstTs, currentTs); |
There was a problem hiding this comment.
This can easily fail if the insertEvent takes more than a second or there is code executed between setting currentTs and calling insert.
This is valid for other tests as well.
Completed Todos. Updated Tests, Removed sleep.
No description provided.