-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-6366] Separate WebSocket max message size into a dedicated REST API #5099
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
base: master
Are you sure you want to change the base?
Conversation
8a0cd93
to
001b3d6
Compare
001b3d6
to
ed1cae5
Compare
I don't know if we should also adapt zeppelin-web? What do you think? |
@Reamer Looks reasonable for zeppelin-web too. Are there specific concerns you’re considering, such as maintenance or behavior differences? |
@seung-00 zeppelin/zeppelin-web/src/app/app.js Line 170 in f1f6713
session_logout and programmatically clicks the login button (.nav-login-btn ).As a result, the login modal pops up immediately when the window opens, which is different from before, and the integration test fails. It looks like we should change this to issue the API request only when the user is authenticated, or defer it until the moment it is actually needed. |
Yes both. All frontends should use the same interfaces. This makes it easier to remove things later on. |
de8726d
to
3e5933e
Compare
@Reamer @tbonelee BTW, if the user isn’t logged in initially, showing the login modal seems more natural. 🤔 |
zeppelin-integration/src/test/java/org/apache/zeppelin/integration/ZeppelinIT.java
Show resolved
Hide resolved
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.
Everything seems right but the unused imports should be removed.
Co-authored-by: ChanHo Lee <[email protected]>
Co-authored-by: ChanHo Lee <[email protected]>
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.
LGTM. I also agree with showing a login modal when authentication is required. Maybe we could address that in another issue.
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 I don’t think we need to do that for every task. Covering both sides would be a bit tricky, since the front-end codebases are quite different.
Sooner or later, we should send the old front end into well-deserved retirement. In my opinion, sooner rather than later.
import org.apache.zeppelin.service.AuthenticationService; | ||
|
||
/** Configurations Rest API Endpoint. */ | ||
@Path("/configurations") |
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.
You have changed the path of the API class here. Probably so that the call is /api/wsMaxMessageSize
. However, I also think /api/configurations/wsMaxMessageSize
is very good and would prefer this API endpoint.
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.
@Reamer
I also think the path you mentioned looks good. However, since the authentication referenced in shiro.ini
appears to be set up based on the /configuration
path, I excluded /configurations
from the path. The wsMaxMessageSize API is intended to be a non-authenticated API.
Please feel free to let me know if you have any other suggestions. 🤔
What is this PR for?
Currently, configuration data is fetched through both REST API and WebSocket channels. However, the WebSocket path does not perform permission checks, and the only required data from it is the WebSocket max message size.
I extracted the websocket max message size field into a dedicated REST API, to improve security and simplify configuration handling.
What type of PR is it?
Improvement
Todos
What is the Jira issue?
How should this be tested?
Screenshots (if appropriate)
Questions: