nginx test #62

dentiny · 2025-08-17T12:23:30Z

Summary

Briefly explain what this PR does.

Related Issues

Closes # or links to related issues.

Changes

Checklist

Code builds correctly
Tests have been added or updated
Documentation updated if necessary
I have reviewed my own changes

gemini-code-assist

Summary of Changes

Hello @dentiny, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

The primary goal of this pull request is to introduce a comprehensive standalone test for the moonlink_service. This test suite validates the service's ability to interact with an Nginx data server, manage table schemas, ingest data, and perform data scans. It ensures the core functionalities of the moonlink_service are working as expected in an integrated environment.

Highlights

New Standalone Test Suite: A new test file src/moonlink_service/src/test.rs has been added, containing test_moonlink_standalone and associated helper functions. This test simulates a full workflow including service startup, table creation, data ingestion via REST API, and data scanning via RPC, interacting with a mock Nginx server.
Dependency Updates: Cargo.lock and src/moonlink_service/Cargo.toml have been updated to include new development dependencies such as reqwest, arrow, parquet, bytes, and tempfile, which are essential for the new testing infrastructure.
Test Utility Export: A new utility function, decode_serialized_read_state_for_testing, has been exposed in the moonlink crate to facilitate the deserialization of read state data for testing purposes.
Service Configuration Enhancements: The ServiceConfig struct in moonlink_service now derives Debug for easier debugging, and the READINESS_PROBE_PORT constant's visibility has been adjusted to pub(crate) to allow access within the crate's test modules.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces tests for the moonlink service, including an integration test that uses nginx. The changes are a good step towards ensuring the service's reliability. However, there are several areas that need improvement.

I've found some leftover debugging println! statements that should be removed. More importantly, the main test function test_moonlink_standalone is currently broken because a helper function read_all_batches is incomplete. I've provided a suggestion to fix it.

Additionally, I've made some suggestions to make the tests more robust, such as using tempfile for temporary directories and improving error handling in test helper functions.

gemini-code-assist · 2025-08-17T12:25:02Z

src/moonlink_service/src/test.rs

+pub async fn read_all_batches(url: &str) -> Vec<RecordBatch> {
+    let resp = reqwest::get(url).await.unwrap();
+    println!("status = {}", resp.status());
+
+    assert!(resp.status().is_success());
+
+    println!("content = {}", resp.text().await.unwrap());
+
+    vec![]
+
+    // let data: Bytes = resp.bytes().await.unwrap();
+    // println!("read len = {}", data.len());
+    // let mut reader = ParquetRecordBatchReaderBuilder::try_new(data).unwrap()
+    //     .build().unwrap();
+
+    // // Collect all record batches
+    // let batches = reader
+    //     .into_iter()
+    //     .map(|b| b.unwrap()) // handle Err properly in production
+    //     .collect();
+
+    // batches
+}


This function is incomplete and returns an empty vector, which causes test_moonlink_standalone to fail. The intended implementation appears to be commented out. The println! statements should also be removed, and unwrap() calls should be replaced with expect() to provide more context on failure.

pub async fn read_all_batches(url: &str) -> Vec<RecordBatch> { let resp = reqwest::get(url) .await .unwrap_or_else(|e| panic!("Failed to GET from url: {}: {}", url, e)); assert!(resp.status().is_success(), "Request to {} failed with status {}", url, resp.status()); let data: Bytes = resp.bytes().await.expect("Failed to read response bytes"); let reader = ParquetRecordBatchReaderBuilder::try_new(data) .expect("Failed to create parquet reader builder") .build() .expect("Failed to build parquet reader"); // Collect all record batches reader .map(|b| b.expect("Failed to read record batch")) .collect() }

gemini-code-assist · 2025-08-17T12:25:02Z

src/moonlink/src/union_read/read_state.rs

+
+        println!("data files = {:?}", data_files);
+
        let remapped_data_files = data_files
            .into_iter()
            .map(|path| read_state_filepath_remap(path))
            .collect::<Vec<_>>();
+
+        println!("remapped data files = {:?}", remapped_data_files);
+
        let remapped_puffin_files = puffin_files
            .into_iter()
            .map(|path| read_state_filepath_remap(path))


These println! statements appear to be for debugging purposes and should be removed before merging.

Suggested change

println!("data files = {:?}", data_files);

let remapped_data_files = data_files

.into_iter()

.map(|path| read_state_filepath_remap(path))

.collect::<Vec<_>>();

println!("remapped data files = {:?}", remapped_data_files);

let remapped_puffin_files = puffin_files

.into_iter()

.map(|path| read_state_filepath_remap(path))

// Map from local filepath to remote file path if needed and if possible.

let remapped_data_files = data_files

.into_iter()

.map(|path| read_state_filepath_remap(path))

.collect::<Vec<_>>();

let remapped_puffin_files = puffin_files

.into_iter()

.map(|path| read_state_filepath_remap(path))

gemini-code-assist · 2025-08-17T12:25:02Z

src/moonlink_service/src/lib.rs

+
+    println!("start with config = {:?}", config);
+
+    println!("sqlite metadata store = {}", config.base_path);
+


These println! statements appear to be for debugging and should be removed.

gemini-code-assist · 2025-08-17T12:25:02Z

src/moonlink_service/src/test.rs

+    recreate_directory(MOONLINK_BACKEND_DIR);
+    let config = ServiceConfig {
+        base_path: MOONLINK_BACKEND_DIR.to_string(),


Instead of using a hardcoded directory and a manual cleanup function, it's better to use tempfile::TempDir for creating a temporary directory for the test. This avoids hardcoded paths, potential conflicts, and ensures the directory is cleaned up automatically. Please also remove the MOONLINK_BACKEND_DIR constant and the recreate_directory function.

Suggested change

recreate_directory(MOONLINK_BACKEND_DIR);

let config = ServiceConfig {

base_path: MOONLINK_BACKEND_DIR.to_string(),

let temp_dir = TempDir::new().expect("Failed to create temp dir");

let config = ServiceConfig {

base_path: temp_dir.path().to_str().expect("temp dir path is not valid utf8").to_string(),

gemini-code-assist · 2025-08-17T12:25:02Z

src/moonlink_service/src/test.rs

+async fn test_readiness_probe() {
+    let url = format!("http://127.0.0.1:{}/ready", READINESS_PROBE_PORT);
+    loop {
+        if let Ok(resp) = reqwest::get(&url).await {
+            if resp.status() == reqwest::StatusCode::OK {
+                return;
+            }
+        }
+        tokio::time::sleep(std::time::Duration::from_millis(100)).await;
+    }
+}


This readiness probe loop can run indefinitely and will silently ignore connection errors, making test failures hard to debug. It's better to have a timeout and to panic with a clear error message if the service doesn't become ready within a reasonable time.

Suggested change

async fn test_readiness_probe() {

let url = format!("http://127.0.0.1:{}/ready", READINESS_PROBE_PORT);

loop {

if let Ok(resp) = reqwest::get(&url).await {

if resp.status() == reqwest::StatusCode::OK {

return;

}

}

tokio::time::sleep(std::time::Duration::from_millis(100)).await;

}

}

async fn test_readiness_probe() {

let url = format!("http://127.0.0.1:{}/ready", READINESS_PROBE_PORT);

for _ in 0..30 { // ~3 seconds timeout

if let Ok(resp) = reqwest::get(&url).await {

if resp.status() == reqwest::StatusCode::OK {

return;

}

}

tokio::time::sleep(std::time::Duration::from_millis(100)).await;

}

panic!("Readiness probe failed to respond after 3 seconds.");

}

gemini-code-assist bot reviewed Aug 17, 2025

View reviewed changes

nginx test

f5f51d2

dentiny force-pushed the hjiang/docker-test branch from a090111 to f5f51d2 Compare August 17, 2025 12:26

dentiny force-pushed the main branch from 2a64a6f to f0c7f22 Compare September 8, 2025 05:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

nginx test #62

nginx test #62

Uh oh!

dentiny commented Aug 17, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Aug 17, 2025

Uh oh!

gemini-code-assist bot Aug 17, 2025

Uh oh!

gemini-code-assist bot Aug 17, 2025

Uh oh!

gemini-code-assist bot Aug 17, 2025

Uh oh!

gemini-code-assist bot Aug 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		println!("start with config = {:?}", config);

		println!("sqlite metadata store = {}", config.base_path);

nginx test #62

Are you sure you want to change the base?

nginx test #62

Uh oh!

Conversation

dentiny commented Aug 17, 2025

Summary

Related Issues

Changes

Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants