DYN-6350 add request timeout for notification service #14564
Conversation
|
@QilongTang I couldn't find where the packageManager request is, would you mind pointing it out to me? Thanks |
|
YAY! |
timeouts for package manager requests are already handled by the package manager client library: |
|
@QilongTang @mjkkirschner I realized that the default timeOut already is 100 seconds, so what would be a suitable value to set? Thanks |
Sorry what do you mean? |
@QilongTang It seems like .net requests already has 100sec timeOut by default. That is the same value we want to set, right? |
If you check the documentation, the unit for this property is millisecond, so default value is 100,000 (100s) which is different than what we are setting here. After checking the link @mjkkirschner sent, I would like to be consistent here so let's set it to 300 like the other Repo config. Thanks! Let me know if you have more questions. |
@QilongTang if we set a timeOut of 300 milliseconds the NotificationCenter component it's not rendering |
…ions request timeOut
|
hi @Enzo707 Would you experiment it and see what would be a proper value for you at least? We can also do some googling together to see the best option here |
Sure, let me see if I can find a way |
| @@ -908,6 +913,7 @@ public PreferenceSettings() | |||
| ReadNotificationIds = new List<string>(); | |||
| DynamoPlayerFolderGroups = new List<DynamoPlayerFolderGroup>(); | |||
| backupLocation = string.Empty; | |||
| NotificationsDefaultTimeOut = 10000; | |||
There was a problem hiding this comment.
hi @Enzo707 Based on my previous comment, would you move this to https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Configuration/Configurations.cs#L14? and not use a property on the current class?
There was a problem hiding this comment.
I dont think we need customers to set this value (as would be the result of current change) so a const value in internal config seems enough to me
There was a problem hiding this comment.
Does it make sense to add NotificationsDefaultTimeOut in Notifications.dll.config so that advanced user will be able to customize the value?
There was a problem hiding this comment.
hi @avidit It may make sense to implement this for all network-related features and expose it in preferences as a whole. I do not see a strong need for our customer to customize the threshold for notifications feature alone. Once the no-network mode has been fully tested and introduced, I think it would put us in a better situation to check network related settings.
…amo Configuration
| /// <summary> | ||
| /// Request timeOut for notifications service | ||
| /// </summary> | ||
| public const int NotificationsDefaultTimeOut = 10000; |
There was a problem hiding this comment.
Ruby defaults to 60 seconds, and .NET defaults to 100s I think the current value is a good one for time saving purpose but not overly aggresive
…calling-notification-service
Purpose
This PR is for setting request timeOut for notification service
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Set a timeOut value of 100 in Dynamo Preference Settings and apply that configuration on notification service request.
Reviewers
@QilongTang
FYIs
@avidit