yuqi1129 commented on PR #12399:
URL: https://github.com/apache/gravitino/pull/12399#issuecomment-5253852709
Thanks for taking this on — the de-duplication is the right call, and the
link cleanup is a real bonus (running the docs link checker against `main`,
this branch removes 16 broken links and adds none).
I pushed two commits to the branch directly rather than leaving them as
suggestions, since they were mechanical. Please review them:
- `e7bef74` fixes a defect in the examples: the catalog-creation and
credential-vending `curl` snippets on all five cloud pages left a trailing
comma on the last property, so the JSON is invalid and the server rejects the
body as copied. Ten snippets. The same commit restores the
`fs.path.config.<name>` placeholder (it had been truncated to `<n>`), realigns
the Storage Backends table after the per-cloud links were added to its first
column, and repoints two anchors that no longer resolve.
- `01c182c` merges current `main`. The only conflict was three links in
`how-to-use-gvfs.md`: both sides had retargeted `security/security.md` (which
`main` has now deleted) to `security/how-to-authenticate.md`, differing only in
link text. I kept your "How to authenticate".
## The main thing: some of the trimming took real content with it
The ~250 lines per page of GVFS/Spark/pandas usage were genuinely duplicated
and should go. But a few things that were cut are per-cloud facts a reader
cannot reconstruct from anywhere else, and they no longer exist anywhere in the
tree.
**1. Which jars the `hadoop fs` path needs, and where they come from.**
Each page now says:
> Add ... to `${HADOOP_HOME}/etc/hadoop/core-site.xml`, then place the same
jars in the Hadoop classpath.
"the same jars" refers back to the Java section, but that section describes
*Maven dependencies*, so the reference doesn't resolve to anything a reader can
copy. What `main` said per cloud:
| Cloud | Removed |
|---|---|
| S3 | `hadoop-aws-${hadoop-version}.jar`, from
`${HADOOP_HOME}/share/hadoop/tools/lib/` |
| OSS | `hadoop-aliyun-*.jar`, `jdom2`, `aliyun-sdk-oss` |
| ADLS | `hadoop-azure-*.jar`, `azure-storage`, `wildfly-openssl` |
| COS | `hadoop-cos-3.3.0-8.3.23.jar` and `cos_api-bundle-5.6.227.jar` —
**explicitly not part of the Apache Hadoop distribution**, must be downloaded
from Maven Central |
The COS note is the one I would most want back. Unlike the others, those
jars are not in the Hadoop distribution, and nothing in the docs hints at that
now.
**2. The Hadoop shell recipe has no configuration example.**
`how-to-use-gvfs.md#via-hadoop-shell-command` is now the only end-to-end CLI
walkthrough, and for configuration it says only:
```shell
# 1. Configure the hadoop `core-site.xml` configuration
# You should put the required properties into this file
```
No XML. The reader has to hand-translate the configuration table into
`core-site.xml` themselves. That section is also written for HDFS (it includes
a Kerberos step), and its jar step copies only
`gravitino-filesystem-hadoop3-runtime`, not a cloud bundle.
Related inconsistency: that section says jars go in
`${HADOOP_HOME}/share/hadoop/common/lib/`, while the cloud pages said
`${HADOOP_HOME}/share/hadoop/tools/lib/`. Only one of those survives now, so
the branch silently settles the question — worth confirming which is correct.
**3. Two removed warnings.**
- The JDK17 `--add-opens=java.base/sun.nio.ch=ALL-UNNAMED` note for Spark
was on all five pages and is now on none. Spark on JDK17 fails without it.
- The "use `s3a://`, not `s3://`, because `s3://` is not supported by
`hadoop-aws`" warning is gone from the S3 page. The examples all use `s3a://`
but no longer say why.
### Suggestion
This can be fixed without undoing the consolidation, roughly 40 lines total:
1. In `how-to-use-gvfs.md`, give the Hadoop shell section a real
`core-site.xml` snippet with the four base properties, and state the jar step
properly. Shared by all backends, written once.
2. In each cloud page's `### Hadoop Command Line`, replace the vague
sentence with that cloud's jar list (2–3 lines). This is the only genuinely
per-cloud part.
3. Put the `s3a://` warning back on the S3 page (one line), and the JDK17
`--add-opens` note in the shared Spark section (one line).
## Smaller points
- **The PR description is stale.** It describes deleting the five cloud
pages, but later commits restored them in trimmed form; only
`fileset-catalog-index.md` is actually deleted now. Since this will likely be
squashed, the description becomes the commit message.
- `s3-endpoint` is `Required: Yes` in the catalog properties table but `No`
in the Python table on the same page. If that difference is real it deserves a
note in the catalog table; if not, the two should agree.
- "Set `credential-providers` on the catalog to one of `gcs-token`" reads
oddly for the single-value cases (GCS, COS, ADLS).
- Two links in `how-to-use-gvfs.md` (lines 81 and 471) point at
`manage-fileset-metadata-using-gravitino.md#manage-filesets-across-multiple-clusters`,
which no longer exists. These predate your PR and I did not want to guess a
replacement target, but they sit in a file you are already touching.
- `how-to-use-gvfs.md` has one misaligned table (the Python configuration
table) that came in from `main` via the merge, not from your work — `#12391`
widened "built-in IDP" to "local user store" without re-padding. Fixing it here
would mix in unrelated changes, so I left it; it probably deserves its own
small PR against `main`.
Happy to write the restored sections myself if you would rather not — just
say so.
--
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]
