-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UI: Take advantage of client request tunneling #3908
UI: Take advantage of client request tunneling #3908
Conversation
Only when the client isn't accessible
// When true, logs cannot be fetched from either the client or the server | ||
noConnection: false, | ||
|
||
clientTimeout: 1000, |
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 still think 1s is ambitious (networking! ¯_(ツ)_/¯ ), but most likely not a huge deal.
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 agree. Especially if the browser attempting the connection is on a slow network or is far away from the server, but it's a careful balance between timing out too early and wasting time before just using the server. It's easy to change for 0.8.1 if need be.
logger: logger('logUrl', 'logParams', function logFetch() { | ||
// If the log request can't settle in one second, the client | ||
// must be unavailable and the server should be used instead | ||
const timing = this.get('useServer') ? this.get('serverTimeout') : this.get('clientTimeout'); |
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.
the added flip flopping is nice 🙆♂️
@@ -10,6 +11,8 @@ import PollLogger from 'nomad-ui/utils/classes/poll-logger'; | |||
|
|||
const MAX_OUTPUT_LENGTH = 50000; | |||
|
|||
export const fetchFailure = url => () => Ember.Logger.warn(`LOG FETCH: Couldn't connect to ${url}`); |
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.
Ember.Logger's is being deprecated and removed (not that it matters here or will happen soon), but console.warn is likely fine here too.
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.
Yeah... Not gonna lie, I just wanted to avoid adding an eslint-ignore.
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.
lol, fair
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.
Looks good! Nice work on the tests too - they're pretty easy to read.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Now that #3892 has landed, the UI can make client API requests through the server when the client isn't accessible from the UI session 🎉
Currently there are only two places this happens in the UI:
These requests now only ever use the server, since the penalty for falling back to the server agent is greater than the cost of always using the server agent.
These requests will try to connect to a client, but if a connection can't be made in 1000ms, the UI retries using the server agent.