chore: update logs + add parallel.for when putting updating tiles to S3#112
chore: update logs + add parallel.for when putting updating tiles to S3#112
Conversation
asafMasa
changed the title
| { | ||
| this._logger.LogDebug($"[{MethodBase.GetCurrentMethod().Name}] start"); | ||
| List<int> zoomLevels = new List<int>(); | ||
| List<int> zoomLevels = new List<int>(Data<IS3Client>.MaxZoomRead); |
There was a problem hiding this comment.
Remove the capacity, it is not nessecary.
There was a problem hiding this comment.
why? isn't it good practice to put list limit?
There was a problem hiding this comment.
I dont think that it is nessecary in this case when we are dealing with small numbers.
If anything, in most cases this will be too much, we will have less zoom levels with the array capacity being 25 (currently).
This also may be a issue if the value is accidently changed to be a smaller value than the amount of zoom levels in a certain data type.
There was a problem hiding this comment.
it's best practice - and we try to stick to best practices
https://stackoverflow.com/questions/25429652/c-sharp-what-is-listt-initial-capacity-used-for
smaller values
MergerCli/appsettings.json
Outdated
| { | ||
| "GENERAL": { | ||
| "validate": false, | ||
| "defaultConnectionLimit": 100, |
There was a problem hiding this comment.
What kind of connection limit? For what? Depending on the answer, this may not be the correct place for this type of configuration.
Remove the 'default' prefix.
In addition, this configuration is not used.
There was a problem hiding this comment.
fixed to servicePointManagerDefaultConnectionLimit
MergerLogic/DataTypes/S3.cs
Outdated
|
|
||
| private async void ParallelUpdateTiles(IEnumerable<Tile> targetTiles) | ||
| { | ||
| await Parallel.ForEachAsync(targetTiles, new ParallelOptions { MaxDegreeOfParallelism = 50 }, async (tile, cancellationToken) => |
There was a problem hiding this comment.
Why is there a '50' hard coded here?
There was a problem hiding this comment.
This was not the question... Why is there a hard coded value here? Why is this not configureable?
In addition, this should be thoroughly checked before being pushed.
There was a problem hiding this comment.
because i think we will never change it...
i'll show you the test results
|
Please thoroughly check the changes you made. |
3 participants
Update logs
Fix version in logs
Add parallel for when putting updating tiles to S3