Copilot commented on code in PR #9379:
URL: https://github.com/apache/gravitino/pull/9379#discussion_r2588950655
##########
docs/iceberg-rest-service.md:
##########
@@ -447,6 +447,14 @@ Gravitino provides the build-in
`org.apache.gravitino.iceberg.common.cache.Local
|---------------------------------------------|--------------------------------------------------------------|---------------|----------|------------------|
| `gravitino.iceberg-rest.extension-packages` | Comma-separated list of
Iceberg REST API packages to expand. | (none) | No |
0.7.0-incubating |
+### Memory settings
+
+The Iceberg REST server uses `GRAVITINO_MEM` for JVM heap/metaspace flags.
Default: `-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m`. The launch script
appends this to `JAVA_OPTS`; set `GRAVITINO_MEM` to change the heap size used
at runtime.
Review Comment:
[nitpick] The terminology for describing how `GRAVITINO_MEM` is used is
inconsistent across the documentation. This section uses "The launch script
appends this to `JAVA_OPTS`", while line 271 in `gravitino-server-config.md`
uses "This value is appended to `JAVA_OPTS` by the launch scripts", and the
Docker image sections use variations like "The launcher appends this" or "This
value is appended by the launcher". Consider using consistent terminology
across all documentation, such as "The launch script appends `GRAVITINO_MEM` to
`JAVA_OPTS`" everywhere.
```suggestion
The Iceberg REST server uses `GRAVITINO_MEM` for JVM heap/metaspace flags.
Default: `-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m`. The launch script
appends `GRAVITINO_MEM` to `JAVA_OPTS`; set `GRAVITINO_MEM` to change the heap
size used at runtime.
```
##########
docs/docker-image-details.md:
##########
@@ -129,6 +137,10 @@ You can deploy the standalone Gravitino Lance REST server
with the Docker image.
docker run --rm -d -p 9102:9102 -e LANCE_REST_GRAVITINO_METALAKE_NAME=test -e
LANCE_REST_PORT=9102 apache/gravitino-lance-rest:latest
```
+Memory settings
+
+Use `GRAVITINO_MEM` to size the JVM (default `-Xms1024m -Xmx1024m
-XX:MaxMetaspaceSize=512m`). Example: `-e GRAVITINO_MEM="-Xms2g -Xmx2g"`. This
value is appended by the launcher, so set `GRAVITINO_MEM` when you need
different heap sizes.
Review Comment:
[nitpick] The example `-e GRAVITINO_MEM="-Xms2g -Xmx2g"` doesn't include the
`-XX:MaxMetaspaceSize` parameter, while the examples for Gravitino server (line
22) and Iceberg REST server (line 80) do include it. For consistency and
completeness, consider including the MaxMetaspaceSize parameter in this example
as well, e.g., `-e GRAVITINO_MEM="-Xms2g -Xmx2g -XX:MaxMetaspaceSize=512m"`.
```suggestion
Use `GRAVITINO_MEM` to size the JVM (default `-Xms1024m -Xmx1024m
-XX:MaxMetaspaceSize=512m`). Example: `-e GRAVITINO_MEM="-Xms2g -Xmx2g
-XX:MaxMetaspaceSize=512m"`. This value is appended by the launcher, so set
`GRAVITINO_MEM` when you need different heap sizes.
```
##########
docs/gravitino-server-config.md:
##########
@@ -266,6 +266,10 @@ Refer to [security](security/security.md) for HTTPS and
authentication configura
|-------------------------------------------|------------------------------------------------------|---------------|----------|---------------|
| `gravitino.metrics.timeSlidingWindowSecs` | The seconds of Gravitino metrics
time sliding window | 60 | No | 0.5.1 |
+### Memory options (GRAVITINO_MEM)
+
+`GRAVITINO_MEM` controls JVM heap/metaspace settings for the Gravitino server,
and is also used by the Iceberg REST server and Lance REST server launchers.
The default is `-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m` (see
`bin/common.sh`). This value is appended to `JAVA_OPTS` by the launch scripts;
set `GRAVITINO_MEM` to change the heap size. Typical values: development
`-Xms1g -Xmx2g`, moderate production `-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g`,
larger deployments `-Xms8g -Xmx8g -XX:MaxMetaspaceSize=1g` or higher depending
on catalog count, plugins, and query concurrency.
+
Review Comment:
[nitpick] This paragraph is very long (over 300 characters in a single line)
and contains multiple inline code examples, making it difficult to read.
Consider breaking it into multiple paragraphs or using a list format similar to
the iceberg-rest-service.md documentation. For example:
```
`GRAVITINO_MEM` controls JVM heap/metaspace settings for the Gravitino
server, and is also used by the Iceberg REST server and Lance REST server
launchers. The default is `-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m` (see
`bin/common.sh`). This value is appended to `JAVA_OPTS` by the launch scripts;
set `GRAVITINO_MEM` to change the heap size.
Typical values:
- Development: `-Xms1g -Xmx2g`
- Moderate production: `-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g`
- Larger deployments: `-Xms8g -Xmx8g -XX:MaxMetaspaceSize=1g` or higher
depending on catalog count, plugins, and query concurrency.
```
```suggestion
`GRAVITINO_MEM` controls JVM heap/metaspace settings for the Gravitino
server, and is also used by the Iceberg REST server and Lance REST server
launchers.
The default is `-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m` (see
`bin/common.sh`). This value is appended to `JAVA_OPTS` by the launch scripts.
Set `GRAVITINO_MEM` to change the heap size.
Typical values:
- Development: `-Xms1g -Xmx2g`
- Moderate production: `-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g`
- Larger deployments: `-Xms8g -Xmx8g -XX:MaxMetaspaceSize=1g` or higher
depending on catalog count, plugins, and query concurrency.
```
##########
docs/docker-image-details.md:
##########
@@ -17,6 +17,10 @@ Container startup commands
docker run --rm -d -p 8090:8090 -p 9001:9001 apache/gravitino:0.7.0-incubating
```
+Memory settings
+
+The JVM heap/metaspace options are controlled by `GRAVITINO_MEM` (default
`-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m`). Override with `-e
GRAVITINO_MEM="-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g"`. The launch script
appends `GRAVITINO_MEM` to `JAVA_OPTS`, so set `GRAVITINO_MEM` when you need
different heap sizes.
Review Comment:
[nitpick] The phrase "when you need different heap sizes" is inconsistent
with similar phrases used in other documentation sections. For example,
`iceberg-rest-service.md` uses "to change the heap size used at runtime" and
`gravitino-server-config.md` uses "to change the heap size". Consider using
consistent phrasing across all sections.
```suggestion
The JVM heap/metaspace options are controlled by `GRAVITINO_MEM` (default
`-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m`). Override with `-e
GRAVITINO_MEM="-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g"`. The launch script
appends `GRAVITINO_MEM` to `JAVA_OPTS`, so set `GRAVITINO_MEM` to change the
heap size used at runtime.
```
##########
conf/gravitino-env.sh.template:
##########
@@ -28,4 +28,4 @@ GRAVITINO_VERSION=GRAVITINO_VERSION_PLACEHOLDER
# export GRAVITINO_HOME
# export GRAVITINO_CONF_DIR
# export GRAVITINO_LOG_DIR # Where log files are stored. PWD by default.
-# export GRAVITINO_MEM # Gravitino jvm mem options Default -Xms1024m
-Xmx1024m -XX:MaxMetaspaceSize=512m
\ No newline at end of file
+# export GRAVITINO_MEM # JVM mem options for Gravitino server, Iceberg
REST server, and Lance REST server. Default: -Xms1024m -Xmx1024m
-XX:MaxMetaspaceSize=512m. This is appended to JAVA_OPTS in the launch scripts,
so set GRAVITINO_MEM if you need to change heap/metaspace sizes.
Review Comment:
[nitpick] The comment wraps to a very long line. Consider breaking it into
multiple lines for better readability. For example:
```
# JVM mem options for Gravitino server, Iceberg REST server, and Lance REST
server.
# Default: -Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m
# This is appended to JAVA_OPTS in the launch scripts, so set GRAVITINO_MEM
if you need to change heap/metaspace sizes.
```
```suggestion
# export GRAVITINO_MEM # JVM mem options for Gravitino server,
Iceberg REST server, and Lance REST server.
# # Default: -Xms1024m -Xmx1024m
-XX:MaxMetaspaceSize=512m
# # This is appended to JAVA_OPTS in the launch
scripts, so set GRAVITINO_MEM if you need to change heap/metaspace sizes.
```
##########
docs/iceberg-rest-service.md:
##########
@@ -447,6 +447,14 @@ Gravitino provides the build-in
`org.apache.gravitino.iceberg.common.cache.Local
|---------------------------------------------|--------------------------------------------------------------|---------------|----------|------------------|
| `gravitino.iceberg-rest.extension-packages` | Comma-separated list of
Iceberg REST API packages to expand. | (none) | No |
0.7.0-incubating |
+### Memory settings
+
+The Iceberg REST server uses `GRAVITINO_MEM` for JVM heap/metaspace flags.
Default: `-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m`. The launch script
appends this to `JAVA_OPTS`; set `GRAVITINO_MEM` to change the heap size used
at runtime.
+Example tuning:
+ - Development: `GRAVITINO_MEM="-Xms1g -Xmx2g"`
+ - Medium workloads: `GRAVITINO_MEM="-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g"`
+ - Heavier concurrency/catalog count: raise heap and metaspace accordingly.
Review Comment:
[nitpick] The bullet points in the example tuning section are indented with
spaces, which may not render correctly in all markdown processors. Standard
markdown bullet points should start at the beginning of the line without
leading spaces. Consider reformatting as:
```
Example tuning:
- Development: `GRAVITINO_MEM="-Xms1g -Xmx2g"`
- Medium workloads: `GRAVITINO_MEM="-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g"`
- Heavier concurrency/catalog count: raise heap and metaspace accordingly.
```
```suggestion
- Development: `GRAVITINO_MEM="-Xms1g -Xmx2g"`
- Medium workloads: `GRAVITINO_MEM="-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g"`
- Heavier concurrency/catalog count: raise heap and metaspace accordingly.
```
##########
docs/gravitino-server-config.md:
##########
@@ -266,6 +266,10 @@ Refer to [security](security/security.md) for HTTPS and
authentication configura
|-------------------------------------------|------------------------------------------------------|---------------|----------|---------------|
| `gravitino.metrics.timeSlidingWindowSecs` | The seconds of Gravitino metrics
time sliding window | 60 | No | 0.5.1 |
+### Memory options (GRAVITINO_MEM)
+
+`GRAVITINO_MEM` controls JVM heap/metaspace settings for the Gravitino server,
and is also used by the Iceberg REST server and Lance REST server launchers.
The default is `-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m` (see
`bin/common.sh`). This value is appended to `JAVA_OPTS` by the launch scripts;
set `GRAVITINO_MEM` to change the heap size. Typical values: development
`-Xms1g -Xmx2g`, moderate production `-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g`,
larger deployments `-Xms8g -Xmx8g -XX:MaxMetaspaceSize=1g` or higher depending
on catalog count, plugins, and query concurrency.
Review Comment:
[nitpick] The terminology for describing how `GRAVITINO_MEM` is used is
inconsistent with other documentation. This section uses "This value is
appended to `JAVA_OPTS` by the launch scripts", while the Iceberg REST
documentation uses "The launch script appends this to `JAVA_OPTS`", and Docker
sections use "The launcher" or "This value is appended by the launcher".
Consider using consistent terminology across all documentation.
```suggestion
`GRAVITINO_MEM` controls JVM heap/metaspace settings for the Gravitino
server, and is also used by the Iceberg REST server and Lance REST server
launchers. The default is `-Xms1024m -Xmx1024m -XX:MaxMetaspaceSize=512m` (see
`bin/common.sh`). The launch script appends this to `JAVA_OPTS`; set
`GRAVITINO_MEM` to change the heap size. Typical values: development `-Xms1g
-Xmx2g`, moderate production `-Xms4g -Xmx4g -XX:MaxMetaspaceSize=1g`, larger
deployments `-Xms8g -Xmx8g -XX:MaxMetaspaceSize=1g` or higher depending on
catalog count, plugins, and query concurrency.
```
##########
docs/docker-image-details.md:
##########
@@ -129,6 +137,10 @@ You can deploy the standalone Gravitino Lance REST server
with the Docker image.
docker run --rm -d -p 9102:9102 -e LANCE_REST_GRAVITINO_METALAKE_NAME=test -e
LANCE_REST_PORT=9102 apache/gravitino-lance-rest:latest
```
+Memory settings
+
+Use `GRAVITINO_MEM` to size the JVM (default `-Xms1024m -Xmx1024m
-XX:MaxMetaspaceSize=512m`). Example: `-e GRAVITINO_MEM="-Xms2g -Xmx2g"`. This
value is appended by the launcher, so set `GRAVITINO_MEM` when you need
different heap sizes.
Review Comment:
[nitpick] The phrase "This value is appended by the launcher" is vague and
inconsistent with other documentation sections. The subject performing the
action is unclear ("by the launcher" vs "by the launch script"). Consider using
consistent and clearer wording that matches the other sections, such as "The
launch script appends `GRAVITINO_MEM` to `JAVA_OPTS`".
```suggestion
Use `GRAVITINO_MEM` to size the JVM (default `-Xms1024m -Xmx1024m
-XX:MaxMetaspaceSize=512m`). Example: `-e GRAVITINO_MEM="-Xms2g -Xmx2g"`. The
launch script appends `GRAVITINO_MEM` to `JAVA_OPTS`, so set `GRAVITINO_MEM`
when you need different heap sizes.
```
##########
docs/docker-image-details.md:
##########
@@ -71,6 +75,10 @@ Container startup commands
docker run --rm -d -p 9001:9001 apache/gravitino-iceberg-rest:0.7.0-incubating
```
+Memory settings
+
+`GRAVITINO_MEM` controls JVM memory (default `-Xms1024m -Xmx1024m
-XX:MaxMetaspaceSize=512m`). Override with `-e GRAVITINO_MEM="-Xms4g -Xmx4g
-XX:MaxMetaspaceSize=1g"`. The launcher appends this to `JAVA_OPTS`, so set
`GRAVITINO_MEM` to change heap sizing.
Review Comment:
[nitpick] The terminology "The launcher appends this to `JAVA_OPTS`" is
inconsistent with the terminology used in other documentation sections. The
Iceberg REST and Gravitino server config docs use "The launch script appends"
or "This value is appended to `JAVA_OPTS` by the launch scripts". Consider
using consistent wording across all sections.
```suggestion
`GRAVITINO_MEM` controls JVM memory (default `-Xms1024m -Xmx1024m
-XX:MaxMetaspaceSize=512m`). Override with `-e GRAVITINO_MEM="-Xms4g -Xmx4g
-XX:MaxMetaspaceSize=1g"`. This value is appended to `JAVA_OPTS` by the launch
scripts, so set `GRAVITINO_MEM` to change heap sizing.
```
--
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]
