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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
