Remove use of global fed metadata within client #878
Conversation
joereuss12
force-pushed
the
cleanup-metadata-client-v2-branch
branch
3 times, most recently
from
343bd34
to
4ae427a
Compare
|
Still has a merge conflict, not super sure why because I thought I fixed that in the last commit but it keeps showing up again. Once this is reviewed I will fix it as long as it remains just |
joereuss12
force-pushed
the
cleanup-metadata-client-v2-branch
branch
5 times, most recently
from
0ece902
to
a4fd1e1
Compare
There was a problem hiding this comment.
Some minor changes requested.
The biggest conceptual problem is the way we use the loader interface is going to cause error messages to be swallowed. We should:
- Ensure that we have unit test coverage, especially around the metadata timeouts, for discovery that ensures a consistent error type.
- Return an error object as part of the cache load and set a shorter lifetime for failure versus success.
client/handle_http_test.go
Outdated
| assert.NoError(t, err) | ||
| if err == nil && len(transferDetailsUpload) == 2 { |
There was a problem hiding this comment.
Why is this all being deleted? If you don't like the repetition, maybe just factor it out to a helper function instead?
There was a problem hiding this comment.
I can't see this because it is on an older commit. I know I change the tests a bit with a later commit so just let me know if it still persists after I commit these fixes.
joereuss12
force-pushed
the
cleanup-metadata-client-v2-branch
branch
from
a4fd1e1
to
54e3b29
Compare
|
@joereuss12 how are we doing here? I saw there's some merge conflicts from the project structure overhaul PR. Do we need more time for another round of review? Are we good with getting this in 7.7 or we need to push it off to 7.8? |
|
@haoming29 This is done, I can patch up the merge conflicts sometime today or early tomorrow. It needs another review and I know @bbockelm said he wanted to be the one to review it but I'm not sure if that is still the case. I would love for this to be in v7.7 because a lot of my current issues rely on this PR being merged in. |
Good to know. Thanks |
This is v2 of PR PelicanPlatform#798 since the merge conflicts were becoming very tedious. Therefore, I took the changes from that PR and converted it over to a new branch. You can get the changes split up a little better from that PR as well but I will do my best to list them here: Changes include: - Added a ttl cache for storing federation url metadata so we do not have to look up each time - Improved unit tests for client commands, Note: for pelican prefix and osdf:// url scheme, it's hard to test since we use osg-htc.org for metadata lookup. Therefore it is hard to work with fed in a box so I just check for the proper metadata lookup - Added function called discoverUrlFederation which discovers a federation from a url and does not populate global metadata config values - Added a function called schemeUnderstood to take some repeated code within client into one function to check for understood url schemes - Added unit tests for discoverUrlFederation and schemeUnderstood
Made the unit test also include downloading a directory within a directory (the inner dir contains a file too), just to expand the unit test further and to test a fail case I found (and fixed) a good while ago.
Solved the pile of current merge conflicts with all the new changes with local cache and origins exporting multiple prefixes
Local cache tests now work with my changes to the client, mostly being that pelican:// url's require a federation discovery url. Also refactors the local cache stat changes to not rely on global metadata
Test was failing due to wacky merging when dealing with merge conflicts. Should work now
changes: - Added unit tests for metadata discover timeouts, cancelling context in metadata discovery, setPreferredPrefix - SetPreferredPrefix is now case-insensitive and will return an error for invalid prefixes - Edits to to unit tests - Moved starting the transferEngine to cmd, this way the cache stays alive for all objects listed for file transfer - Added the cache to the TransferEngine object - Made the ttl cache wrapped in a suppressed loader to avoid multiple concurrent requests from the same process - Fixed bug where we were swallowing the error sent back from discovering metadata, it is not in the cacheItem object sent back from the cache so we can check for that error and return it - Edited PelicanUrl, only contains directorUrl in the object now - Edited debug/error logging messages
All that was needed was allowing an empty string for setPreferredPrefix
joereuss12
force-pushed
the
cleanup-metadata-client-v2-branch
branch
from
b61db17
to
5ce04d5
Compare
Most significant change to the original set of work is to revert API interface breakage in the client. Instead of passing the transfer engine in explicitly, make an internal reference. For now, don't expose the TransferEngine object outside the helper functions (`DoStat`, `DoGet`, `DoPut`); this may be revisited at some point in the future but I don't want to expose the implementation details yet.
There was a problem hiding this comment.
Did all my review comments as a pushed commit to the branch to hurry things along.
Once tests are passing, I think it's ready to go if:
- Create a follow-on ticket to remove the use of
DiscoverFederationin the client sharing URL code. In fact, it looks like that has some of the exact same URL parsing code for the director -- may be able to eliminate quite a bit there. - Create a follow-on ticket to switch the
config.GetPreferredPrefix/config.SetPreferredPrefixobject to use the newly-exportedConfigPrefixtype instead of astring. I started looking at that refactor here and it just packs one too many things into the PR.
This is v2 of PR #798 since the merge conflicts were becoming very tedious. Therefore, I took the changes from that PR and converted it over to a new branch. You can get the changes split up a little better from that PR as well but I will do my best to list them here:
Changes include: