[Improvement] GVFS makes duplicated REST calls on every filesystem operation
- Dominant language
- Java
- Stars
- 3.2k
- Forks
- 935
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 298
Description
### What would you like to be improved?
When there is an operation related to GVFS, there are a lot of duplicated and unnecessary REST communications between GVFS client and the Gravitino server.
## Details
Every `GravitinoVirtualFileSystem` operation issues **11 REST calls** to obtain **5 distinct pieces of information**. Measured on current `main` against `DefaultGVFSOperations`, with the metadata cache at its default (disabled), counting the requests the server actually received:
| Operation | total | loadCatalog | loadFileset | loadSchema | getSecrets | getFileLocation |
|---|---|---|---|---|---|---|
| `open` | 11 | **4** | **2** | 1 | **3** | 1 |
| `create` | 11 | **4** | **2** | 1 | **3** | 1 |
| `append` | 11 | **4** | **2** | 1 | **3** | 1 |
| `delete` | 11 | **4** | **2** | 1 | **3** | 1 |
| `mkdirs` | 11 | **4** | **2** | 1 | **3** | 1 |
| `getFileStatus` | 11 | **4** | **2** | 1 | **3** | 1 |
| `listStatus` | 11 | **4** | **2** | 1 | **3** | 1 |
| `getDefaultBlockSize` | 11 | **4** | **2** | 1 | **3** | 1 |
| `getDefaultReplication` | 11 | **4** | **2** | 1 | **3** | 1 |
| `setWorkingDirectory` | 11 | **4** | **2** | 1 | **3** | 1 |
| `rename` | 13 | **5** | **2** | 1 | **3** | 2 |
The same catalog is loaded four times and the same fileset twice to serve one operation. These are independent lookups: no caller knows another one already asked, so each repeats the same GET. Note that `getFilesetCatalog()` is itself `client.loadCatalog(name).asFilesetCatalog()`, and `asFilesetCatalog()` returns `this` — one object implementing both `Catalog` and `FilesetCatalog`, so `[1]` and `[3]` are literally the same request for the same object.
```
DefaultGVFSOperations.open(path) everything below is BaseGVFSOperations
│
├── getActualFileSystem(path) -> getActualFileSystemByLocationName(...)
│ │
│ ├── getFileset(ident)
│ │ ├── getFilesetCatalog(...) [1] loadCatalog
│ │ └── loadFileset(...) [2] loadFileset
│ │
│ ├── getAllProperties(ident)
│ │ ├── getGravitinoClient().loadCatalog(...) [3] loadCatalog == same GET as [1]
│ │ ├── catalog.supportsSecrets().getSecrets() [4] getSecrets
│ │ ├── catalog.asSchemas().loadSchema(...) [5] loadSchema
│ │ ├── schema.supportsSecrets().getSecrets() [6] getSecrets
│ │ ├── catalog.asFilesetCatalog().loadFileset(...) [7] loadFileset == same GET as [2]
│ │ └── fileset.supportsSecrets().getSecrets() [8] getSecrets
│ │
│ └── createFilesetLocationIfNeed(ident, fs, path)
│ └── getFilesetCatalog(...) [9] loadCatalog == same GET as [1]
│
└── getActualFilePath(path)
├── getFilesetCatalog(...) [10] loadCatalog == same GET as [1]
└── getFileLocation(...) [11] getFileLocation
```
Three separate causes:
1. **The two halves of a path resolution do not share a lookup.** Every operation calls `getActualFileSystem` and `getActualFilePath` back to back, and each resolves the catalog from scratch, although both need the same one.
2. **`getAllProperties` re-fetches what its caller already holds.** It reloads the catalog and the fileset the caller just resolved, and it reaches `getGravitinoClient()` directly rather than going through the cache-aware `getFilesetCatalog()` / `getFileset()` / `getSchema()`. That last part matters: **these calls are not eliminated even when `fs.gravitino.filesetMetadataCache.cache.enable` is turned on.**
3. **Properties are built eagerly for a cache that almost always hits.** Building the property map costs a schema load plus three `getSecrets()` calls — and `getSecrets()` is a REST call per metadata object that no cache absorbs. But the map is only read when the `FileSystem` cache misses, which happens once per scheme/authority/user per JVM. Every operation after the first pays four REST calls for a map it immediately discards.
The cost scales with the number of filesystem operations, not the amount of metadata. Reading a fileset that holds many files re-resolves the same unchanged catalog once per file (we have seen more than 10,000 REST calls for a single pass over the data).
## How should we improve?
Resolve each piece of information once per operation and pass it down.
| Operation | AS-IS | TO-BE (cache off) | TO-BE (cache on) |
|---|---|---|---|
| `open` / `create` / `delete` / `listStatus` / ... | 11 | **3** | **1** |
| `rename` | 13 | **5** | **2** |
```
DefaultGVFSOperations.open(path) everything below is BaseGVFSOperations
│
└── resolvePath(path) new; resolves the catalog once and passes it down
├── getFilesetCatalog(...) [1] loadCatalog
├── getFileset(ident, catalog) [2] loadFileset
│
├── buildFileSystem(ident, catalog, fileset, ...) reuses [1] and [2]
│ └── on FileSystem cache miss only:
│ loadSchema, getSecrets x3, credentials
│
└── buildActualFilePath(ident, catalog, ...) reuses [1]
└── getFileLocation(...) [3] getFileLocation
```
`buildFileSystem` and `buildActualFilePath` are private extractions of the bodies that `getActualFileSystemByLocationName` and `getActualFilePath` already had. Nothing is removed: both public methods keep their signature and behaviour, and now call the extracted body after resolving the catalog themselves. The extraction is what lets `resolvePath` call the same body with a catalog it already holds, instead of each half resolving its own.
```
AS-IS getActualFileSystem(path) resolves a catalog, then builds the FileSystem
getActualFilePath(path) resolves a catalog, then builds the path
-> a caller needing both resolves the catalog twice
TO-BE getActualFileSystem(path) resolves a catalog -> buildFileSystem(catalog, ...)
getActualFilePath(path) resolves a catalog -> buildActualFilePath(catalog, ...)
resolvePath(path) resolves a catalog -> both builders <- new
-> a caller needing both uses resolvePath and resolves it once
```
Concretely:
- Add a single `resolvePath()` that returns both the `FileSystem` and the resolved storage path, so the catalog is resolved once per operation instead of once per consumer.
- Have `getAllProperties`, `getSchema`, `createFilesetLocationIfNeed` and `getCredentialProperties` accept the already-resolved `Catalog` and `Fileset` instead of fetching them again, and route through the cache-aware accessors so enabling the metadata cache actually removes them.
- Split the property map in two: the small set that forms the `FileSystem` cache key, built from values already in hand, and the full set(schema properties and all secrets) deferred behind a `Supplier` that only runs when a `FileSystem` is genuinely constructed. This removes the per-operation `loadSchema` and all three `getSecrets()` calls.
Secrets are deliberately kept out of the cache-key half: no `FileSystemProvider` derives an authority from a secret (`getFullAuthority` reads only the principal and impersonation keys), and each `getSecrets()` is a round trip.
Property precedence is unchanged (catalog → schema → fileset → filesystem conf → `fs.path.config.*`), and no configuration is added. `getFileLocation` remains one call per operation; reducing *that* is a separate concern, since it also carries the per-file audit event.
While making this change we also found that `getSchema()` reads the `filesetMetadataCache` field directly rather than the lazy `getFilesetMetadataCache()` accessor that every other lookup uses. The field is null until first access, so `getSchema()` throws an NPE on any path that reaches it before the cache is initialized. Currently nothing reaches it, because `getAllProperties` loads the schema itself; routing through `getSchema()` exposes it.
A regression test asserts the catalog is loaded exactly once per operation, it fails with 4 on current `main`.
I would be happy to open a PR for this if the approach looks reasonable.
Contributor guide
Research direction
Start with DefaultGVFSOperations and the shared methods in BaseGVFSOperations, especially getActualFileSystemByLocationName, getActualFilePath, getAllProperties, getSchema, createFilesetLocationIfNeed, and getCredentialProperties. Follow the existing cache-aware accessors and the described resolvePath flow, including the lazy metadata-cache initialization issue. Run the regression test that asserts catalog loads once per operation; done means preserving public behavior while meeting the stated REST-call counts.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100