Cosmos DB for Events#61
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| public class EventItem : IEvent | ||
| { | ||
| public EventItem() {} |
There was a problem hiding this comment.
style: Consider removing the empty constructor if it's not explicitly needed
| } | ||
|
|
||
| [JsonPropertyName("id")] | ||
| public string Id { get; set; } |
There was a problem hiding this comment.
style: Consider making Id property init-only to prevent modification after creation
| [JsonPropertyName("saId")] | ||
| public Guid? ServiceAccountId { get; set; } | ||
| [JsonPropertyName("domain")] | ||
| public string DomainName { get; set; } |
There was a problem hiding this comment.
style: DomainName should be nullable (string?) for consistency with other properties
| private readonly Container _container; | ||
|
|
||
| public EventRepository(GlobalSettings globalSettings) | ||
| : this("TODO") |
There was a problem hiding this comment.
logic: Replace 'TODO' with actual connection string retrieval from globalSettings
| _database = _client.GetDatabase("events"); | ||
| _container = _database.GetContainer("events"); |
There was a problem hiding this comment.
style: Consider using configuration for database and container names instead of hardcoding
| // TODO: How should we handle the partition yet? Perhaps something like table storage did with | ||
| // orgId, userId, providerId |
There was a problem hiding this comment.
logic: Implement a proper partitioning strategy before production use
| public Task CreateManyAsync(IEnumerable<IEvent> events) | ||
| { | ||
| // ref: https://learn.microsoft.com/en-us/azure/cosmos-db/nosql/tutorial-dotnet-bulk-import | ||
| var tasks = new List<Task>(); | ||
| foreach (var e in events) | ||
| { | ||
| tasks.Add(CreateAsync(e)); | ||
| } | ||
| return Task.WhenAll(tasks); | ||
| } |
There was a problem hiding this comment.
style: Consider using Cosmos DB bulk operations for better performance when creating multiple events
| var result = new PagedResult<IEvent>(); | ||
| while (iterator.HasMoreResults) | ||
| { | ||
| var response = await iterator.ReadNextAsync(); | ||
| result.Data.AddRange(response); | ||
| if (response.Count > 0) | ||
| { | ||
| result.ContinuationToken = response.ContinuationToken; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: This pagination implementation may not handle large result sets efficiently. Consider implementing server-side pagination
| private async Task ExecuteAsync(CancellationToken cancellationToken) | ||
| { | ||
| var storageConnectionString = _configuration["azureStorageConnectionString"]; | ||
| var cosmosConnectionString = _configuration["cosmosConnectionString"]; |
There was a problem hiding this comment.
style: Consider using a more descriptive configuration key, such as 'cosmosDbConnectionString' for clarity
| IEventRepository repo = string.IsNullOrWhiteSpace(cosmosConnectionString) ? | ||
| new Core.Repositories.TableStorage.EventRepository(storageConnectionString) : | ||
| new Core.Repositories.Cosmos.EventRepository(cosmosConnectionString); |
There was a problem hiding this comment.
style: This logic might be better placed in a factory method or dependency injection setup
3 participants
Type of change
Objective
Code changes
Before you submit
dotnet format --verify-no-changes) (required)Greptile Summary
This pull request introduces Cosmos DB support for event handling in the server application, focusing on the implementation of a new EventItem class and EventRepository for Cosmos DB integration.
src/Core/Models/Data/EventItem.cswith a newEventItemclass for Cosmos DB event data handlingsrc/Core/Repositories/Cosmos/EventRepository.csfor managing events in Cosmos DBsrc/EventsProcessor/AzureQueueHostedService.csto support Cosmos DB alongside existing TableStoragesrc/SharedWeb/Utilities/ServiceCollectionExtensions.csto register Cosmos DB EventRepository