Improvement: reuse the tcp connection when plan files

[chenzl25]

Considering I am trying to read an iceberg table from S3. Currently, plan_files() seems unable to reuse the TCP connection for HTTP requests. It will lead to a relatively high latency. I am not sure whether it is a good practice to enable Connection: keep-alive by default, what do you think?

[liurenjie1024]

Considering I am trying to read an iceberg table from S3. Currently, plan_files() seems unable to reuse the TCP connection for HTTP requests. It will lead to a relatively high latency. I am not sure whether it is a good practice to enable Connection: keep-alive by default, what do you think?

I think the buffering is handles by opendal? cc @Xuanwo

[Xuanwo]

Hi, OpenDAL handles those connection-related tasks (by reqwest).

Currently, FileIO builds new operators every time:

iceberg-rust/crates/iceberg/src/io/storage.rs

Lines 104 to 119 in 4083f81

	#[cfg(feature = "storage-s3")]
	Storage::S3 { scheme_str, config } => {
	let op = super::s3_config_build(config, path)?;
	let op_info = op.info();
	// Check prefix of s3 path.
	let prefix = format!("{}://{}/", scheme_str, op_info.name());
	if path.starts_with(&prefix) {
	Ok((op, &path[prefix.len()..]))
	} else {
	Err(Error::new(
	ErrorKind::DataInvalid,
	format!("Invalid s3 url: {}, should start with {}", path, prefix),
	))
	}
	}

I think we can improve this by using the same HTTP client instead.

[chenzl25]

Great, Looking forward to it!

[chenzl25]

I have tested duckdb and clickhouse to scan an iceberg table in MinIO as well. They can reuse a TCP connection to send HTTP requests. BTW, clickhouse can reuse connections aggressively.

DuckDB:

Clickhouse:

[chenzl25]

However, risingwave the database use iceberg-rust seems can't reuse any TCP connection. You can see that every HTTP request is in its own TCP connection.

[Xuanwo]

Thanks for the sharing. I believe this can be addressed by reusing the same http client.

[Xuanwo]

We will need apache/opendal#4967 for this. I'm working on it now.

[chenzl25]

We will need apache/opendal#4967 for this. I'm working on it now.

Which opendal's version do we need to bump into?

[Xuanwo]

Which opendal's version do we need to bump into?

I'm guessing it will be included in our next release 0.49.

[chenzl25]

I have already verified that using a global HTTP client works in this issue and the performance improvement is impressive (about 10 times faster: from 1500+ms to 150+ms).
https://github.com/risingwavelabs/iceberg-rust/blob/24bd5869f2779a8b9786b5a6e1f9723844f5a82c/crates/iceberg/src/io.rs#L438-L440

[Xuanwo]

Also, cc @sdd, who is focusing on the iceberg benchmark now.

[sdd]

I also have some local code that reuses the same OpenDAL operator rather than creating a new one each time. I'd not submitted it yet as I wasn't sure of the validity of doing that in every possible scenario but it has been working well for me

[Xuanwo]

I also have some local code that reuses the same OpenDAL operator rather than creating a new one each time. I'd not submitted it yet as I wasn't sure of the validity of doing that in every possible scenario but it has been working well for me

Great! I will address apache/opendal#4967 first, then consider the best approach for iceberg-rust.