Skip to content

51425 Cache Daily batches#80

Merged
barrie-cooper merged 65 commits intomainfrom
Feature/51425-caching-fss-response-for-daily-batches
Jul 28, 2022
Merged

51425 Cache Daily batches#80
barrie-cooper merged 65 commits intomainfrom
Feature/51425-caching-fss-response-for-daily-batches

Conversation

@ganpat-tech
Copy link
Copy Markdown
Collaborator

Coding for daily batches
Unit Tests

Ganpat Gawde and others added 30 commits July 5, 2022 19:20
… and retrieve the cache data that is not expiry AB#51192 AB#51101
@ganpat-tech ganpat-tech requested review from a team and rockydevnet as code owners July 22, 2022 13:40
@barrie-cooper barrie-cooper changed the title Feature/51425 caching fss response for daily batches 51425 Cache FSS response for daily batches Jul 22, 2022
@barrie-cooper barrie-cooper changed the title 51425 Cache FSS response for daily batches 51425 Cache Daily batches Jul 26, 2022
Copy link
Copy Markdown
Contributor

@usha13529 usha13529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments to address please.

{
_logger.LogInformation(EventIds.FSSDailyBatchFilesResponseStoreToCacheStart.ToEventId(), "Request for storing file share service daily NM files response in azure table storage is started for with _X-Correlation-ID:{correlationId}", correlationId);

await _fileShareServiceCache.InsertCacheObject(searchResult, rowKey, _cacheConfiguration.Value.FssCacheResponseTableName, frequency, correlationId);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note InsertCacheObject method is updated to support other frequency types under 51427 Cache Cumulative batches #77. This call need to update once PR77 merge into main.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for highlighting, Yes i will do that.

@ganpat-tech ganpat-tech requested a review from usha13529 July 26, 2022 13:57
Copy link
Copy Markdown
Contributor

@usha13529 usha13529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ganpat-tech. Looks good to me.

@barrie-cooper barrie-cooper requested a review from Chownder July 28, 2022 11:41
Copy link
Copy Markdown
Contributor

@barrie-cooper barrie-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor points

Copy link
Copy Markdown
Contributor

@barrie-cooper barrie-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @ganpat-tech

Copy link
Copy Markdown
Contributor

@Chownder Chownder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, many thanks.

There is one comment about a potential refactoring NMDataService (quite a bit of code duplication), but given that the code itself is sound this isn't necessary.

}

public async Task<List<ShowDailyFilesResponseModel>> GetDailyBatchDetailsFiles(string correlationId)
public async Task<ShowDailyFilesResponseListModel> GetDailyBatchDetailsFiles(string correlationId)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of shared code between this method and GetWeeklyBatchFiles (and possibly getAllYearWeek) - this would be a good candidate for a refactor.

However, given time limits (and one more of these change due for cumulatives), possibly not the best time to start one.

Copy link
Copy Markdown
Contributor

@barrie-cooper barrie-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @ganpat-tech

@barrie-cooper barrie-cooper merged commit 54ee6e8 into main Jul 28, 2022
@barrie-cooper barrie-cooper deleted the Feature/51425-caching-fss-response-for-daily-batches branch July 28, 2022 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants