gemini-code-assist[bot] commented on code in PR #39142:
URL: https://github.com/apache/beam/pull/39142#discussion_r3488971650
##########
sdks/python/apache_beam/yaml/integration_tests.py:
##########
@@ -556,6 +561,75 @@ def temp_oracle_database():
yield f"jdbc:oracle:thin:system/oracle@localhost:{port}/XEPDB1"
[email protected]
+def temp_iceberg_table_with_pk(table_data):
+
+ # Create a temp dir that will be shared between host and container.
+ # We use the exact same path on both to avoid path mapping issues.
+ # We create it in the current working directory (workspace) because
+ # Docker in GitHub Actions often cannot mount directories from /tmp.
+ temp_dir = tempfile.mkdtemp(dir=os.getcwd())
+ os.chmod(temp_dir, 0o777)
+
+ # Start the Iceberg REST catalog container
+ container = DockerContainer("tabulario/iceberg-rest:0.6.0")
+ container.with_exposed_ports(8181)
+ container.with_volume_mapping(temp_dir, temp_dir, mode='rw')
+ container.with_env("HADOOP_USER_NAME", "iceberg")
+ container.with_env("CATALOG_WAREHOUSE", temp_dir)
+ container.with_env(
+ "CATALOG_IO__IMPL", "org.apache.iceberg.hadoop.HadoopFileIO")
+
+ try:
+ container.start()
+
+ ip = container.get_container_host_ip()
+ port = container.get_exposed_port(8181)
+ api_url = f"http://{ip}:{port}";
+
+ # Poll the REST API until it is ready
+ for _ in range(30):
+ try:
+ response = requests.get(f"{api_url}/v1/config", timeout=5)
+ if response.status_code == 200:
+ break
+ except (requests.exceptions.ConnectionError,
requests.exceptions.Timeout):
+ pass
+ time.sleep(1)
+ else:
+ raise RuntimeError("Iceberg REST catalog failed to start in time.")
+
+ # Create namespace 'db'
+ requests.post(
+ f"{api_url}/v1/namespaces",
+ json={"namespace": ["db"]},
+ headers={"Content-Type": "application/json"},
+ timeout=10)
+
+ # Create table with primary key
+ response = requests.post(
+ f"{api_url}/v1/namespaces/db/tables",
+ json=table_data,
+ headers={"Content-Type": "application/json"},
+ timeout=10)
+ if response.status_code != 200:
+ raise RuntimeError(f"Failed to create Iceberg table: {response.text}")
+
+ # Change permissions of the created directories inside the container
+ # so the host user can write to them.
+ container.get_wrapped_container().exec_run(f"chmod -R 777 {temp_dir}")
+
+ yield {
+ "api_url": api_url,
+ "temp_dir": temp_dir,
+ "table": f"db.{table_data['name']}"
+ }
+
+ finally:
+ container.stop()
+ shutil.rmtree(temp_dir, ignore_errors=True)
Review Comment:
During the pipeline execution (which happens inside the `yield` block), new
files and metadata may be written to the shared `temp_dir` by the container.
Since these files are created by the container (often running as root), they
may have restrictive permissions that prevent the host user from deleting them.
This can leave behind root-owned files in the CI workspace, even with
`ignore_errors=True` in `shutil.rmtree`.\n\nTo ensure clean teardown, we should
run `chmod -R 777` inside the container in the `finally` block before stopping
the container.
```python
finally:\n try:\n
container.get_wrapped_container().exec_run([\"chmod\", \"-R\", \"777\",
temp_dir])\n except Exception:\n pass\n container.stop()\n
shutil.rmtree(temp_dir, ignore_errors=True)
```
##########
sdks/python/apache_beam/yaml/integration_tests.py:
##########
@@ -556,6 +561,75 @@ def temp_oracle_database():
yield f"jdbc:oracle:thin:system/oracle@localhost:{port}/XEPDB1"
[email protected]
+def temp_iceberg_table_with_pk(table_data):
+
+ # Create a temp dir that will be shared between host and container.
+ # We use the exact same path on both to avoid path mapping issues.
+ # We create it in the current working directory (workspace) because
+ # Docker in GitHub Actions often cannot mount directories from /tmp.
+ temp_dir = tempfile.mkdtemp(dir=os.getcwd())
+ os.chmod(temp_dir, 0o777)
+
+ # Start the Iceberg REST catalog container
+ container = DockerContainer("tabulario/iceberg-rest:0.6.0")
+ container.with_exposed_ports(8181)
+ container.with_volume_mapping(temp_dir, temp_dir, mode='rw')
+ container.with_env("HADOOP_USER_NAME", "iceberg")
+ container.with_env("CATALOG_WAREHOUSE", temp_dir)
+ container.with_env(
+ "CATALOG_IO__IMPL", "org.apache.iceberg.hadoop.HadoopFileIO")
+
+ try:
+ container.start()
+
+ ip = container.get_container_host_ip()
+ port = container.get_exposed_port(8181)
+ api_url = f"http://{ip}:{port}";
+
+ # Poll the REST API until it is ready
+ for _ in range(30):
+ try:
+ response = requests.get(f"{api_url}/v1/config", timeout=5)
+ if response.status_code == 200:
+ break
+ except (requests.exceptions.ConnectionError,
requests.exceptions.Timeout):
+ pass
+ time.sleep(1)
+ else:
+ raise RuntimeError("Iceberg REST catalog failed to start in time.")
+
+ # Create namespace 'db'
+ requests.post(
+ f"{api_url}/v1/namespaces",
+ json={"namespace": ["db"]},
+ headers={"Content-Type": "application/json"},
+ timeout=10)
Review Comment:
The response of the namespace creation request is not checked. If the
namespace creation fails, the subsequent table creation will fail with a less
clear error. It is safer to check the response status code and raise an error
if it fails.
```python
response = requests.post(\n f"{api_url}/v1/namespaces",\n
json={\"namespace\": [\"db\"]},\n headers={\"Content-Type\":
\"application/json\"},\n timeout=10)\n if response.status_code not in
(200, 201, 204):\n raise RuntimeError(f\"Failed to create namespace:
{response.text}\")
```
##########
sdks/python/apache_beam/yaml/integration_tests.py:
##########
@@ -556,6 +561,75 @@ def temp_oracle_database():
yield f"jdbc:oracle:thin:system/oracle@localhost:{port}/XEPDB1"
[email protected]
+def temp_iceberg_table_with_pk(table_data):
+
+ # Create a temp dir that will be shared between host and container.
+ # We use the exact same path on both to avoid path mapping issues.
+ # We create it in the current working directory (workspace) because
+ # Docker in GitHub Actions often cannot mount directories from /tmp.
+ temp_dir = tempfile.mkdtemp(dir=os.getcwd())
+ os.chmod(temp_dir, 0o777)
+
+ # Start the Iceberg REST catalog container
+ container = DockerContainer("tabulario/iceberg-rest:0.6.0")
+ container.with_exposed_ports(8181)
+ container.with_volume_mapping(temp_dir, temp_dir, mode='rw')
+ container.with_env("HADOOP_USER_NAME", "iceberg")
+ container.with_env("CATALOG_WAREHOUSE", temp_dir)
+ container.with_env(
+ "CATALOG_IO__IMPL", "org.apache.iceberg.hadoop.HadoopFileIO")
+
+ try:
+ container.start()
+
+ ip = container.get_container_host_ip()
+ port = container.get_exposed_port(8181)
+ api_url = f"http://{ip}:{port}";
+
+ # Poll the REST API until it is ready
+ for _ in range(30):
+ try:
+ response = requests.get(f"{api_url}/v1/config", timeout=5)
+ if response.status_code == 200:
+ break
+ except (requests.exceptions.ConnectionError,
requests.exceptions.Timeout):
+ pass
+ time.sleep(1)
+ else:
+ raise RuntimeError("Iceberg REST catalog failed to start in time.")
+
+ # Create namespace 'db'
+ requests.post(
+ f"{api_url}/v1/namespaces",
+ json={"namespace": ["db"]},
+ headers={"Content-Type": "application/json"},
+ timeout=10)
+
+ # Create table with primary key
+ response = requests.post(
+ f"{api_url}/v1/namespaces/db/tables",
+ json=table_data,
+ headers={"Content-Type": "application/json"},
+ timeout=10)
+ if response.status_code != 200:
+ raise RuntimeError(f"Failed to create Iceberg table: {response.text}")
+
+ # Change permissions of the created directories inside the container
+ # so the host user can write to them.
+ container.get_wrapped_container().exec_run(f"chmod -R 777 {temp_dir}")
Review Comment:
Using a string with `exec_run` can lead to issues if the path contains
spaces or special characters (e.g., if the workspace path has spaces). It is
safer to pass the command as a list of arguments to avoid shell parsing issues.
```suggestion
# Change permissions of the created directories inside the container\n
# so the host user can write to them.\n
container.get_wrapped_container().exec_run([\"chmod\", \"-R\", \"777\",
temp_dir])
```
--
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]
