-
-
Notifications
You must be signed in to change notification settings - Fork 323
Update support for iRODS #17341
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?
Update support for iRODS #17341
Conversation
irods/bin/src/test/resources/iRODS (iPlant Collaborative).cyberduckprofile
Show resolved
Hide resolved
irods/src/main/java/ch/cyberduck/core/irods/IRODSCopyFeature.java
Outdated
Show resolved
Hide resolved
irods/src/main/java/ch/cyberduck/core/irods/IRODSDeleteFeature.java
Outdated
Show resolved
Hide resolved
irods/src/main/java/ch/cyberduck/core/irods/IRODSUploadFeature.java
Outdated
Show resolved
Hide resolved
irods/src/main/java/ch/cyberduck/core/irods/IRODSUploadFeature.java
Outdated
Show resolved
Hide resolved
irods/src/test/java/ch/cyberduck/core/irods/IRODSExceptionMappingServiceTest.java
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Discussed in #14449 (comment) |
|
TODO: Update documentation for iRODS - https://docs.cyberduck.io/protocols/irods/ |
|
updating the docs is a separate repo, would have its own PR |
This comment was marked as resolved.
This comment was marked as resolved.
660266c to
f914720
Compare
|
Rebased PR on top of master. Verified the following:
With this PR, users will not be able to calculate checksums on data in an iRODS zone. Cyberduck will report checksums in their hex form if they exist in iRODS. We could add an option to the iRODS profile which allows users to instruct Cyberduck to calculate checksums following an upload, but that didn't feel like the correct approach, mainly because profiles seem to be loaded once on program start and never reread. As for the unit/integration tests, I'm not sure how the implementation can be tested without a real iRODS server. Finally, this PR bumps the minimum iRODS version requirement to 4.3.2. A new version of irods4j will be needed before this is merged. See irods/irods4j#126. Will get a new version released |
irods/src/main/java/ch/cyberduck/core/irods/IRODSUploadFeature.java
Outdated
Show resolved
Hide resolved
irods/src/main/java/ch/cyberduck/core/irods/IRODSUploadFeature.java
Outdated
Show resolved
Hide resolved
irods/src/main/java/ch/cyberduck/core/irods/IRODSUploadFeature.java
Outdated
Show resolved
Hide resolved
|
@dkocher When I move a file in or out of a directory, I see a new iRODS connection get created. It disappears eventually, but not cleanly. Can you explain why Cyberduck creates a new connection every time a file is moved? The move class is very similar to other implementations so it's not clear to me what causes the new connection. |
You will need to return |
|
@dkocher When would someone choose stateless or stateful? To provide some context, a single iRODS connection cannot be used to execute multiple API operations in parallel. A single request is sent and the server returns a response. |
|
If you can share credentials with us for your test environment we can run daily integration tests from our CI. |
We will have to keep it stateful then as otherwise it will be attempted to use a single connection for multiple actions in parallel, i.e. when the user is browsing folders or a file transfer with multiple files in parallel. I will need to review how we can still support the native copy feature implementation for iRODS that would not require a new connection. |
We don't have a CI system for people to hook into yet. We have a small set of tools which make it easy to launch one or more iRODS servers for testing. It's likely overkill for your needs though. With that said, building a containerized environment for testing iRODS is pretty easy. I'm happy to put together a Docker compose project for the iRODS component. That will allow you to launch it on a local computer and have it available for testing.
So, because the If that's true, why is it that the Move operation results in connections which do not disconnect? No other operation shares that behavior. I added some log statements to my local build to try and box in what is leading to the additional connections, but it didn't help. However, it did reveal a high number of instantiations of |
|
We have other usages of Docker Compose containers in integration tests, thus that should be feasible. |
|
Where should I place the Docker Compose project? Is the test directory for irods appropriate? I figure you can move things around if needed. |
|
@dkocher What triggers the Copy implementation? Using the Duplicate option within the context menu doesn't appear to trigger it. |
|
Docker compose project added under test directory. Squashing everything down. |
That is correct. Connections are leased from a pool.
No, a second connection will be opened. |
| container = new ComposeContainer( | ||
| new File(IRODSDockerComposeManager.class.getResource("/docker/docker-compose.yml").getFile())) | ||
| .withPull(false) | ||
| .withLocalCompose(true) |
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.
Will need to be removed when rebasing with #17640 merged.
| .withLocalCompose(true) |
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.
Hmm, the tests fail when that line is removed. They pass when it is included.
Is that expected?
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.
Have you rebased to origin/master? With the updated testcontainers:2.x dependency the API should no longer be available.
I'm probably misunderstanding, but wouldn't I need to expand multiple folders fast enough such that the operations overlap? And if that's the case, is there a trick to doing that in the GUI? |
Select multiple folders and press the right arrow key to expand. |
|
Wow, can't believe I forgot about multi-select. Will give that a try. |
…it exception handling logic
04272c0 to
c85a33f
Compare
| static { | ||
| IRODSApi.setApplicationName(new PreferencesUseragentProvider().get()); | ||
| // TODO: Use "new PreferencesUseragentProvider().get()" once irods/irods#8733 is resolved. | ||
| IRODSApi.setApplicationName("Cyberduck"); |
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.
Please use
| IRODSApi.setApplicationName("Cyberduck"); | |
| IRODSApi.setApplicationName(PreferencesFactory.get().getProperty("application.name")); |
as the library is not limited to usages in Cyberduck.
| final IRODSApi.ConnectionOptions options = new IRODSApi.ConnectionOptions(); | ||
|
|
||
| options.clientServerNegotiation = preferences.getProperty(IRODSProtocol.CLIENT_SERVER_NEGOTIATION); | ||
| options.trustManagers = new TrustManager[]{session.getFeature(X509TrustManager.class)}; |
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.
[ERROR] /home/runner/work/cyberduck/cyberduck/irods/src/main/java/ch/cyberduck/core/irods/IRODSConnectionUtils.java:[69,16] cannot find symbol
[ERROR] symbol: variable trustManagers
[ERROR] location: variable options of type org.irods.irods4j.low_level.api.IRODSApi.ConnectionOptions
Is there a dependency version update required?
This PR updates support for iRODS by replacing Jargon (legacy iRODS library) with irods4j.
The foundational work was implemented by @MINGYJ, a recent iRODS intern. My commits are mainly polish and corrections around the use of the irods4j library.
Basic functionality is working - i.e. single stream uploads/downloads, renames, editing, etc.
The parallel transfer implementation doesn't appear to be working as intended. This is likely due to not having a full understanding of how the Cyberduck components fit together - i.e. Read/WriteFeature vs Upload/DownloadFeature.
Here are the steps for performing parallel transfer (i.e. multipart uploads) in iRODS.
Putting in draft for now. Feedback and guidance on how to implement proper support for parallel transfer would be greatly appreciated.
Resolves #14449.