You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "renechoi (via GitHub)" <gi...@apache.org> on 2025/07/15 15:13:58 UTC
[PR] [MINOR] Improve Repository class stability and validation [zeppelin]
renechoi opened a new pull request, #4977:
URL: https://github.com/apache/zeppelin/pull/4977
- Add comprehensive input validation for Repository ID, URL, credentials, and proxy settings
- Introduce RepositoryException for better error handling and clear error messages
- Enhance JSON deserialization with proper exception handling and validation
- Add extensive test coverage with 21 test cases covering all validation scenarios
- Improve JavaDoc documentation with usage examples and exception conditions
- Maintain backward compatibility while strengthening internal validation
### What is this PR for?
A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - https://zeppelin.apache.org/contribution/contributions.html
### What type of PR is it?
Bug Fix
Improvement
Feature
Documentation
Hot Fix
Refactoring
*Please leave your type of PR only*
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN/
* Put link here, and add [ZEPPELIN-*Jira number*] in PR title, eg. [ZEPPELIN-533]
### How should this be tested?
* Strongly recommended: add automated unit tests for any new or changed behavior
* Outline any manual steps to test the PR here.
### Screenshots (if appropriate)
### Questions:
* Does the license files need to update?
* Is there breaking changes for older versions?
* Does this needs documentation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "ankursaini2006 (via GitHub)" <gi...@apache.org>.
ankursaini2006 commented on code in PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#discussion_r2234833981
##########
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java:
##########
Review Comment:
@Reamer yes I confirm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MINOR] Improve Repository class stability and validation [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3077103700
Looked at the changes. Looks pretty good. I have not yet lighted all the corners. Can you make it so that the repository class variables are final?
An extension of the builder pattern could be helpful here, where a build() is called at the end.
btw. the changes are definitely not ‘minor’. Please create a JIRA ticket.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer merged PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "ankursaini2006 (via GitHub)" <gi...@apache.org>.
ankursaini2006 commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3089568156
Hi @renechoi
First of all thank you for the changes you made.
Recently I raised a ticket [ZEPPELIN-6231](https://issues.apache.org/jira/browse/ZEPPELIN-6231) for highlighting a bug that I found while testing Zeppelin on Kubernetes.
After reviewing the code changes you made it appears that the bug will be fixed.
Thanks
Ankur
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "renechoi (via GitHub)" <gi...@apache.org>.
renechoi commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3092361873
@Reamer Thank you for the feedback! I've removed all the deprecated methods as requested.
Regarding the org.eclipse.aether package usage, I investigated the codebase and found that:
1. The Repository class serves as a bridge between Zeppelin's serializable repository configuration and Maven Aether's RemoteRepository
2. Key usage points are:
- InterpreterSettingManager: Uses getRepositories() method to convert Repository to RemoteRepository for dependency resolution
- InterpreterRestApi: Exposes repository information through REST endpoints
- The conversion methods (toRemoteRepository() and fromRemoteRepository()) are the main integration points
The current design allows Zeppelin to maintain a clean separation where:
- Internal repository configuration uses our Repository class (JSON serializable)
- Actual Maven dependency resolution uses Aether's RemoteRepository
I agree that reducing the exposure of org.eclipse.aether classes would improve the architecture. As you suggested, this could be a good follow-up improvement after this PR is merged.
@ankursaini2006 Thank you for confirming that these changes should fix the issue you reported in ZEPPELIN-6231! It's great to hear that this PR will help resolve the Kubernetes deployment issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "renechoi (via GitHub)" <gi...@apache.org>.
renechoi commented on code in PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#discussion_r2232359309
##########
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java:
##########
@@ -309,9 +313,14 @@ private void loadFromFile() throws IOException {
}
if (infoSaving.interpreterRepositories != null) {
- for (RemoteRepository repo : infoSaving.interpreterRepositories) {
- if (!dependencyResolver.getRepos().contains(repo)) {
+ for (Repository repo : infoSaving.interpreterRepositories) {
+ // Check if repository already exists to avoid duplicates
+ boolean exists = this.interpreterRepositories.stream()
+ .anyMatch(r -> r.getId().equals(repo.getId()));
+ if (!exists) {
Review Comment:
@Reamer Thank you for the feedback!
I've updated the code as requested:
- When loading from file, existing repositories with the same ID are now removed and overwritten (last one wins)
- The `addRepository()` method already had this behavior, so now both file loading and API calls behave consistently
- Tests have been updated to reflect the new behavior
@ankursaini2006 Thanks for catching this inconsistency! The behavior is now unified between file loading and API calls.
I agree that refactoring to use `Map<String, Repository>` would be a good improvement for a follow-up PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "renechoi (via GitHub)" <gi...@apache.org>.
renechoi commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3138732166
@Reamer Hi, just wanted to gently check in on this PR. Please let me know if there is anything else needed from my side for the merge. Thanks again for your time and reviews!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3142557920
@renechoi
I've just been a little busy with other things over the last few days.
Thank you very much for your contribution. Merged into master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "ankursaini2006 (via GitHub)" <gi...@apache.org>.
ankursaini2006 commented on code in PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#discussion_r2229317298
##########
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java:
##########
Review Comment:
Hello
The expected behavior should be to override the default repository with the repository being loaded from the saved interpreters.json
I have already raised this [bug](https://issues.apache.org/jira/browse/ZEPPELIN-6232) and there is already an opened merge request.
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "renechoi (via GitHub)" <gi...@apache.org>.
renechoi commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3113599171
@ankursaini2006 Thank you for your insights!
Regarding ZEPPELIN-6231: Great to hear that these Repository improvements should fix the Kubernetes deployment issue you reported.
About the dependency resolution performance issue you raised - this is indeed a critical performance problem. I investigated the code and confirmed that `DependencyResolver.getArtifactsWithDep()` always downloads the entire transitive dependency tree using `system.resolveDependencies()`. Your experience with 20 minutes for downloading trino jdbc jar is a serious concern.
Your suggestion to use `resolveArtifact()` instead of `resolveDependencies()` makes sense for cases where only the main artifact is needed. However, this would require careful implementation to avoid runtime ClassNotFoundException issues.
@Reamer What do you think about creating a follow-up JIRA ticket for this dependency resolution optimization? We could:
1. Add a configuration option to control transitive dependency resolution
2. Implement a `resolveWithoutTransitive` flag in DependencyResolver
3. Allow users to choose between full dependency tree vs. single artifact download
This would be a significant performance improvement while maintaining backward compatibility. Should we track this as a separate enhancement after this PR is merged?
Ready for your review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on code in PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#discussion_r2234817826
##########
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java:
##########
Review Comment:
I haven't tested the code manually, but the latest changes should have implemented the requirement. Can you confirm this for me?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3082818303
It looks much better with the builder pattern. :+1:
Please remove the methods that you have marked with @deprecated. We will only merge the feature into the master and do not need backward compatibility.
I'm not entirely satisfied with the changes yet, but it's a good first attempt.
In my opinion, we should not use classes from the `org.eclipse.aether` package in the other Zeppelin modules (zeppelin-zengine, zeppelin-server).
I don't know if this is even feasible, which is why I would merge these changes first and then look into this issue.
Based on initial findings, everything hinges on the method `public List<RemoteRepository> getRepositories()` in the class `InterpreterSettingManager`.
However, the UI, which needs to display the JSON object correctly, could also be involved here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "renechoi (via GitHub)" <gi...@apache.org>.
renechoi commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3114300203
I've fixed the CI failure.
The issue was in `InterpreterSettingManager.loadFromFile()` where repositories were being added without checking for duplicates, causing the test to fail (expected 2 repositories but got 3).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3113799287
Currently CI fails because of your changes.
```
Error: Failures:
Error: InterpreterSettingManagerTest.testInitInterpreterSettingManager:85 expected: <2> but was: <3>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3125814197
I will merge the pull request on Wednesday if no further comments are received.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "renechoi (via GitHub)" <gi...@apache.org>.
renechoi commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3078438252
@Reamer Thank you for the review!
I've created a JIRA ticket as suggested: https://issues.apache.org/jira/browse/ZEPPELIN-6245
Regarding your suggestions:
1. Final variables: I've updated the Repository class to make the instance variables final. This will indeed improve immutability and thread safety.
2. Builder pattern with build(): Great suggestion! I did refactor to implement a proper builder pattern with a build() method that performs final validation and returns an immutable Repository instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3113768377
I don't think that's a good idea. If you don't need the transitive dependencies, you can exclude them just like you would in Maven.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on code in PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#discussion_r2231757167
##########
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java:
##########
@@ -309,9 +313,14 @@ private void loadFromFile() throws IOException {
}
if (infoSaving.interpreterRepositories != null) {
- for (RemoteRepository repo : infoSaving.interpreterRepositories) {
- if (!dependencyResolver.getRepos().contains(repo)) {
+ for (Repository repo : infoSaving.interpreterRepositories) {
+ // Check if repository already exists to avoid duplicates
+ boolean exists = this.interpreterRepositories.stream()
+ .anyMatch(r -> r.getId().equals(repo.getId()));
+ if (!exists) {
Review Comment:
Good catch @ankursaini2006
@renechoi Please delete the existing repository and overwrite it with the repository loaded by the file.
If all repositories have the same Id, the last one wins.
Btw. I think the data structure here is wrong. It would be better to choose a Map<Id, Repository> here, then there would be no duplicates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "ankursaini2006 (via GitHub)" <gi...@apache.org>.
ankursaini2006 commented on code in PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#discussion_r2231213742
##########
zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java:
##########
Review Comment:
@Reamer I believe this bug should be fixed in this PR otherwise there will be two different behaviours. One when loading from the file (it will keep the default repositories) and one when a new repository is created with existing id via API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6245] Improve Repository class stability and validation [zeppelin]
Posted by "ankursaini2006 (via GitHub)" <gi...@apache.org>.
ankursaini2006 commented on PR #4977:
URL: https://github.com/apache/zeppelin/pull/4977#issuecomment-3092376137
@renechoi since you are refactoring the dependencies management, I have one question. if we request an artifact from maven repository, currently whole dependency tree is downloaded and eventually the requested dependency is downloaded. It takes a lot of time. During my test for downloading trino jdbc jar, it took 20mins. Why don’t we just download the requested artifact itself ? Instead of doing collect dependencies why don’t we perform resolve artifact ?
source: [resolveDependencies](https://github.com/apache/zeppelin/blob/25cd340f1d831a38d41270684169a480379cec7a/zeppelin-interpreter/src/main/java/org/apache/zeppelin/dep/DependencyResolver.java#L183)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org