-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load balance mode #62
Conversation
} | ||
} | ||
|
||
// If we found the last successful client,we'll rotate the list so that the next client is at the beginning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If we found the last successful client,we'll rotate the list so that the next client is at the beginning | |
// If we found the last successful client, we'll rotate the list so that the next client is at the beginning |
1a38b87
to
3f10c09
Compare
|
||
// If we found the last successful client,we'll rotate the list so that the next client is at the beginning | ||
if nextClientIndex < len(clients) { | ||
clients = append(clients[nextClientIndex:], clients[:nextClientIndex]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have concern about this, this would be executed in super high frequency, almost every time of executing the ExecuteFailoverPolicy()
right? It would create new array every time, have to allocate a new memory space to accommodate the new array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can think of a more effective way to do the array rotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array rotation is executed every time when ExecuteFailoverPolicy
called, I don't think the frequency will be super high. It depends on the refresh interval the customer set and if load balance mode enabled. Besides, the number of replicas usually will not reach hundreds, O(n) time complexity is acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it may have many AzureAppConfigurationProvider yaml resources in one cluster to reconcile, and the improvement would be super easy to implement, so I think it's worth making a change.
No description provided.