Xuanwo commented on code in PR #1400:
URL: https://github.com/apache/iceberg-rust/pull/1400#discussion_r2135460063


##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -167,37 +151,103 @@ impl<'a> Transaction<'a> {
     }
 
     /// Creates replace sort order action.
-    pub fn replace_sort_order(self) -> ReplaceSortOrderAction<'a> {
+    pub fn replace_sort_order(self) -> ReplaceSortOrderAction {
         ReplaceSortOrderAction {
-            tx: self,
             sort_fields: vec![],
         }
     }
 
-    /// Remove properties in table.
-    pub fn remove_properties(mut self, keys: Vec<String>) -> Result<Self> {
-        self.apply(
-            vec![TableUpdate::RemoveProperties { removals: keys }],
-            vec![],
-        )?;
-        Ok(self)
-    }
-
     /// Set the location of table
-    pub fn set_location(mut self, location: String) -> Result<Self> {
-        self.apply(vec![TableUpdate::SetLocation { location }], vec![])?;
-        Ok(self)
+    pub fn update_location(&mut self) -> UpdateLocationAction {
+        UpdateLocationAction::new()
     }
 
     /// Commit transaction.
-    pub async fn commit(self, catalog: &dyn Catalog) -> Result<Table> {
+    pub async fn commit(&mut self, catalog: Arc<&dyn Catalog>) -> 
Result<Table> {
+        if self.actions.is_empty() {
+            // nothing to commit
+            return Ok(self.base_table.clone());
+        }
+
+        let tx = self.clone();
+        (|mut tx: Transaction| async {
+            let result = tx.do_commit(catalog.clone()).await;
+            (tx, result)
+        })
+        .retry(
+            ExponentialBuilder::new()
+                // TODO retry strategy should be configurable
+                .with_min_delay(Duration::from_millis(100))
+                .with_max_delay(Duration::from_millis(60 * 1000))
+                .with_total_delay(Some(Duration::from_millis(30 * 60 * 1000)))
+                .with_max_times(4)
+                .with_factor(2.0)
+                .build(),
+        )
+        .context(tx)
+        .sleep(tokio::time::sleep)
+        .when(|e| {
+            if let Some(err) = e.downcast_ref::<Error>() {

Review Comment:
   We need to use `downcast_ref` because `do_commit` returns an 
`anyhow::Error`. If we update the API for `do_commit` to return an 
`iceberg::Error` instead, I believe it would make things clearer.



-- 
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

Reply via email to