pithecuse527 opened a new issue, #12856:
URL: https://github.com/apache/gravitino/issues/12856
### 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. Where they
come from:
```
DefaultGVFSOperations.open(path)
│
├── getActualFileSystem(path) -> getActualFileSystemByLocationName(...)
│ │
│ ├── getFileset(ident)
│ │ ├── getFilesetCatalog(...) [1] loadCatalog
│ │ └── loadFileset(...) [2] loadFileset
│ │
│ ├── getAllProperties(ident)
│ │ ├── getGravitinoClient().loadCatalog(...) [3] loadCatalog
<- already at [1]
│ │ ├── catalog.supportsSecrets().getSecrets() [4] getSecrets
│ │ ├── catalog.asSchemas().loadSchema(...) [5] loadSchema
│ │ ├── schema.supportsSecrets().getSecrets() [6] getSecrets
│ │ ├── catalog.asFilesetCatalog().loadFileset(...) [7] loadFileset
<- already at [2]
│ │ └── fileset.supportsSecrets().getSecrets() [8] getSecrets
│ │
│ └── createFilesetLocationIfNeed(ident, fs, path)
│ └── getFilesetCatalog(...) [9] loadCatalog
<- already at [1]
│
└── getActualFilePath(path)
├── getFilesetCatalog(...) [10] loadCatalog
<- already at [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)
│
└── resolvePath(path) one catalog lookup, shared by
both halves
├── 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
```
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. One-line fix,
included.
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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
