You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Xuanwo (via GitHub)" <gi...@apache.org> on 2024/01/26 09:32:46 UTC
[PR] feat: Bump hive_metastore to use pure rust thrift impl `volo` [iceberg-rust]
Xuanwo opened a new pull request, #174:
URL: https://github.com/apache/iceberg-rust/pull/174
Close https://github.com/apache/iceberg-rust/pull/155
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Bump hive_metastore to use pure rust thrift impl `volo` [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #174:
URL: https://github.com/apache/iceberg-rust/pull/174#discussion_r1477279307
##########
crates/catalog/hms/src/catalog.rs:
##########
@@ -71,19 +54,29 @@ impl Debug for HmsCatalog {
impl HmsCatalog {
/// Create a new hms catalog.
pub fn new(config: HmsCatalogConfig) -> Result<Self> {
- let mut channel = thrift::transport::TTcpChannel::new();
- channel
- .open(config.address.as_str())
- .map_err(from_thrift_error)?;
- let (i_chan, o_chan) = channel.split().map_err(from_thrift_error)?;
- let i_chan = TBufferedReadTransport::new(i_chan);
- let o_chan = TBufferedWriteTransport::new(o_chan);
- let i_proto = TBinaryInputProtocol::new(i_chan, true);
- let o_proto = TBinaryOutputProtocol::new(o_chan, true);
- let client = ThriftHiveMetastoreSyncClient::new(i_proto, o_proto);
+ let address = config
+ .address
+ .as_str()
+ .to_socket_addrs()
+ .map_err(from_io_error)?
+ .next()
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!("invalid address: {}", config.address),
+ )
+ })?;
+
+ let client = ThriftHiveMetastoreClientBuilder::new("hms")
+ .address(address)
+ // Framed thrift rpc is not enabled by default in HMS, we use
+ // buffered instead.
+ .make_codec(volo_thrift::codec::default::DefaultMakeCodec::buffered())
Review Comment:
I agree we should make this setting available to users, though I believe it's a lower priority. I've created an issue at https://github.com/apache/iceberg-rust/issues/188 to keep track of this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Bump hive_metastore to use pure rust thrift impl `volo` [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #174:
URL: https://github.com/apache/iceberg-rust/pull/174
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Bump hive_metastore to use pure rust thrift impl `volo` [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #174:
URL: https://github.com/apache/iceberg-rust/pull/174#discussion_r1477279054
##########
crates/catalog/hms/src/utils.rs:
##########
@@ -15,13 +15,28 @@
// specific language governing permissions and limitations
// under the License.
+use anyhow::anyhow;
use iceberg::{Error, ErrorKind};
+use std::fmt::Debug;
+use std::io;
/// Format a thrift error into iceberg error.
-pub fn from_thrift_error(error: thrift::Error) -> Error {
+pub fn from_thrift_error<T>(error: volo_thrift::error::ResponseError<T>) -> Error
+where
+ T: Debug,
+{
Error::new(
ErrorKind::Unexpected,
"operation failed for hitting thrift error".to_string(),
)
+ .with_source(anyhow!("thrift error: {:?}", error))
+}
+
+/// Format an io error into iceberg error.
+pub fn from_io_error(error: io::Error) -> Error {
Review Comment:
I'm not sure if moving to core and making it a public API is a good idea. What if we address this once we have the same `from_io_error` in core?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Bump hive_metastore to use pure rust thrift impl `volo` [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #174:
URL: https://github.com/apache/iceberg-rust/pull/174#discussion_r1468272268
##########
crates/catalog/hms/src/catalog.rs:
##########
@@ -71,19 +54,29 @@ impl Debug for HmsCatalog {
impl HmsCatalog {
/// Create a new hms catalog.
pub fn new(config: HmsCatalogConfig) -> Result<Self> {
- let mut channel = thrift::transport::TTcpChannel::new();
- channel
- .open(config.address.as_str())
- .map_err(from_thrift_error)?;
- let (i_chan, o_chan) = channel.split().map_err(from_thrift_error)?;
- let i_chan = TBufferedReadTransport::new(i_chan);
- let o_chan = TBufferedWriteTransport::new(o_chan);
- let i_proto = TBinaryInputProtocol::new(i_chan, true);
- let o_proto = TBinaryOutputProtocol::new(o_chan, true);
- let client = ThriftHiveMetastoreSyncClient::new(i_proto, o_proto);
+ let address = config
+ .address
+ .as_str()
+ .to_socket_addrs()
+ .map_err(from_io_error)?
+ .next()
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!("invalid address: {}", config.address),
+ )
+ })?;
+
+ let client = ThriftHiveMetastoreClientBuilder::new("hms")
+ .address(address)
+ // Framed thrift rpc is not enabled by default in HMS, we use
+ // buffered instead.
+ .make_codec(volo_thrift::codec::default::DefaultMakeCodec::buffered())
Review Comment:
Maybe it's better to make this a config?
##########
crates/catalog/hms/Cargo.toml:
##########
@@ -28,13 +28,9 @@ license = { workspace = true }
keywords = ["iceberg", "hive", "catalog"]
[dependencies]
+anyhow = { workspace = true }
async-trait = { workspace = true }
-hive_metastore = "0.0.1"
+hive_metastore = "0.0.2"
iceberg = { workspace = true }
-# the thrift upstream suffered from no regular rust release.
-#
-# [test-rs](https://github.com/tent-rs) is an organization that helps resolves this
-# issue. And [tent-thrift](https://github.com/tent-rs/thrift) is a fork of the thrift
-# crate, built from the thrift upstream with only version bumped.
-thrift = { package = "tent-thrift", version = "0.18.1" }
typed-builder = { workspace = true }
+volo-thrift = "0.9.2"
Review Comment:
How about moving this and `hive_metastore` to workspace?
##########
crates/catalog/hms/src/utils.rs:
##########
@@ -15,13 +15,28 @@
// specific language governing permissions and limitations
// under the License.
+use anyhow::anyhow;
use iceberg::{Error, ErrorKind};
+use std::fmt::Debug;
+use std::io;
/// Format a thrift error into iceberg error.
-pub fn from_thrift_error(error: thrift::Error) -> Error {
+pub fn from_thrift_error<T>(error: volo_thrift::error::ResponseError<T>) -> Error
+where
+ T: Debug,
+{
Error::new(
ErrorKind::Unexpected,
"operation failed for hitting thrift error".to_string(),
)
+ .with_source(anyhow!("thrift error: {:?}", error))
+}
+
+/// Format an io error into iceberg error.
+pub fn from_io_error(error: io::Error) -> Error {
Review Comment:
`io::Error` is a common error, how about moving it to core crate?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Bump hive_metastore to use pure rust thrift impl `volo` [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #174:
URL: https://github.com/apache/iceberg-rust/pull/174#discussion_r1477279307
##########
crates/catalog/hms/src/catalog.rs:
##########
@@ -71,19 +54,29 @@ impl Debug for HmsCatalog {
impl HmsCatalog {
/// Create a new hms catalog.
pub fn new(config: HmsCatalogConfig) -> Result<Self> {
- let mut channel = thrift::transport::TTcpChannel::new();
- channel
- .open(config.address.as_str())
- .map_err(from_thrift_error)?;
- let (i_chan, o_chan) = channel.split().map_err(from_thrift_error)?;
- let i_chan = TBufferedReadTransport::new(i_chan);
- let o_chan = TBufferedWriteTransport::new(o_chan);
- let i_proto = TBinaryInputProtocol::new(i_chan, true);
- let o_proto = TBinaryOutputProtocol::new(o_chan, true);
- let client = ThriftHiveMetastoreSyncClient::new(i_proto, o_proto);
+ let address = config
+ .address
+ .as_str()
+ .to_socket_addrs()
+ .map_err(from_io_error)?
+ .next()
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!("invalid address: {}", config.address),
+ )
+ })?;
+
+ let client = ThriftHiveMetastoreClientBuilder::new("hms")
+ .address(address)
+ // Framed thrift rpc is not enabled by default in HMS, we use
+ // buffered instead.
+ .make_codec(volo_thrift::codec::default::DefaultMakeCodec::buffered())
Review Comment:
Tracked at https://github.com/apache/iceberg-rust/issues/188
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Bump hive_metastore to use pure rust thrift impl `volo` [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #174:
URL: https://github.com/apache/iceberg-rust/pull/174#issuecomment-1925703005
cc @liurenjie1024, please review again. Sorry for the late.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Bump hive_metastore to use pure rust thrift impl `volo` [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #174:
URL: https://github.com/apache/iceberg-rust/pull/174#issuecomment-1920810766
cc @Xuanwo Any updates?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Bump hive_metastore to use pure rust thrift impl `volo` [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #174:
URL: https://github.com/apache/iceberg-rust/pull/174#discussion_r1477278925
##########
crates/catalog/hms/Cargo.toml:
##########
@@ -28,13 +28,9 @@ license = { workspace = true }
keywords = ["iceberg", "hive", "catalog"]
[dependencies]
+anyhow = { workspace = true }
async-trait = { workspace = true }
-hive_metastore = "0.0.1"
+hive_metastore = "0.0.2"
iceberg = { workspace = true }
-# the thrift upstream suffered from no regular rust release.
-#
-# [test-rs](https://github.com/tent-rs) is an organization that helps resolves this
-# issue. And [tent-thrift](https://github.com/tent-rs/thrift) is a fork of the thrift
-# crate, built from the thrift upstream with only version bumped.
-thrift = { package = "tent-thrift", version = "0.18.1" }
typed-builder = { workspace = true }
+volo-thrift = "0.9.2"
Review Comment:
Fixed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org