laskoviymishka commented on code in PR #1572:
URL: https://github.com/apache/iceberg-go/pull/1572#discussion_r3711437595
##########
catalog/hive/hive.go:
##########
@@ -150,7 +150,13 @@ func (c *Catalog) ListTables(ctx context.Context,
namespace table.Identifier) it
for _, tableName := range tableNames {
tbl, err := c.client.GetTable(ctx, database, tableName)
if err != nil {
- continue
+ if isNoSuchObjectError(err) {
+ continue
+ }
+
+ yield(nil, fmt.Errorf("failed to load table
%s.%s while listing: %w", database, tableName, err))
Review Comment:
Small consistency thing: Glue and REST both yield `table.Identifier{}` on
the error path and here we yield `nil`. Callers shouldn't be dereferencing the
identifier after a non-nil error anyway so it's harmless, but I'd match the
other catalogs.
##########
catalog/hive/hive.go:
##########
@@ -150,7 +150,13 @@ func (c *Catalog) ListTables(ctx context.Context,
namespace table.Identifier) it
for _, tableName := range tableNames {
tbl, err := c.client.GetTable(ctx, database, tableName)
if err != nil {
- continue
+ if isNoSuchObjectError(err) {
Review Comment:
`isNoSuchObjectError` is now the whole ballgame here: it's the only thing
standing between "skip a concurrently deleted table" and "abort the listing
with a real error." The problem is it matches on the substrings `"not found"`
and `"does not exist"`, and plenty of operational failures carry those (a DNS
miss like `"metastore host not found"`, an auth `"credentials not found"`, or
anything wrapping our own `catalog.ErrNoSuchTable`, whose message is `"table
does not exist"`). Each of those gets classified as a concurrent deletion and
silently skipped, which is the exact bug this PR is trying to kill.
In the `GetTable` path gohive only ever returns typed Thrift exceptions, so
I'd lead with a type check and let that carry the decision:
```go
var nsoe *hive_metastore.NoSuchObjectException
if errors.As(err, &nsoe) {
continue
}
```
If we want to keep a string fallback for other callers I'd narrow it to just
`"NoSuchObjectException"` and drop the two broad ones. Same gate applies on the
`ListViews` side. wdyt?
##########
catalog/hive/hive_test.go:
##########
@@ -280,6 +280,39 @@ func TestHiveListTablesEmpty(t *testing.T) {
mockClient.AssertExpectations(t)
}
+func TestHiveListTablesHandlesObjectLoadingErrors(t *testing.T) {
Review Comment:
The two subtests are each single-purpose: one only skips, one only
propagates. The case I'd actually worry about is the mixed one, since that's
where the new branch ordering can go wrong: a listing where a NoSuchObject
entry and an operational-failure entry both appear, plus a good one after. I'd
add a subtest with `[deleted (skip), broken (propagate), good]` and assert the
broken error surfaces while the good one's `GetTable` is never reached. A
cancelled-context case that propagates would be a nice one to have too. (Same
goes for the `ListViews` test.)
##########
catalog/hive/hive_test.go:
##########
@@ -280,6 +280,39 @@ func TestHiveListTablesEmpty(t *testing.T) {
mockClient.AssertExpectations(t)
}
+func TestHiveListTablesHandlesObjectLoadingErrors(t *testing.T) {
+ t.Run("skips concurrently deleted tables", func(t *testing.T) {
+ mockClient := &mockHiveClient{}
+ mockClient.On("GetTables", mock.Anything, "test_database",
"*").Return([]string{"deleted", "test_table"}, nil).Once()
+ mockClient.On("GetTable", mock.Anything, "test_database",
"deleted").Return(nil, errNoSuchObject).Once()
+ mockClient.On("GetTable", mock.Anything, "test_database",
"test_table").Return(testIcebergHiveTable1, nil).Once()
+
+ var tables []table.Identifier
+ for identifier, err := range NewCatalogWithClient(mockClient,
nil).ListTables(context.Background(), DatabaseIdentifier("test_database")) {
+ require.NoError(t, err)
+ tables = append(tables, identifier)
+ }
+ require.Equal(t,
[]table.Identifier{TableIdentifier("test_database", "test_table")}, tables)
+ mockClient.AssertExpectations(t)
+ })
+
+ t.Run("propagates operational failures", func(t *testing.T) {
+ loadErr := errors.New("metastore unavailable")
+ mockClient := &mockHiveClient{}
+ mockClient.On("GetTables", mock.Anything, "test_database",
"*").Return([]string{"broken", "test_table"}, nil).Once()
+ mockClient.On("GetTable", mock.Anything, "test_database",
"broken").Return(nil, loadErr).Once()
+
+ var gotErr error
+ for _, err := range NewCatalogWithClient(mockClient,
nil).ListTables(context.Background(), DatabaseIdentifier("test_database")) {
+ gotErr = err
+ }
+ require.ErrorIs(t, gotErr, loadErr)
+ require.ErrorContains(t, gotErr, "failed to load table
test_database.broken while listing")
+ mockClient.AssertNotCalled(t, "GetTable", mock.Anything,
"test_database", "test_table")
Review Comment:
`AssertNotCalled` with those exact args doesn't really check what we want:
there's no `On` registered for `"test_table"`, so any call would already blow
up as unexpected. It also doesn't prove iteration actually stopped. I'd swap it
for `mockClient.AssertNumberOfCalls(t, "GetTable", 1)`, which pins that we
stopped after the broken entry.
##########
catalog/hive/hive_test.go:
##########
@@ -280,6 +280,39 @@ func TestHiveListTablesEmpty(t *testing.T) {
mockClient.AssertExpectations(t)
}
+func TestHiveListTablesHandlesObjectLoadingErrors(t *testing.T) {
+ t.Run("skips concurrently deleted tables", func(t *testing.T) {
+ mockClient := &mockHiveClient{}
+ mockClient.On("GetTables", mock.Anything, "test_database",
"*").Return([]string{"deleted", "test_table"}, nil).Once()
+ mockClient.On("GetTable", mock.Anything, "test_database",
"deleted").Return(nil, errNoSuchObject).Once()
+ mockClient.On("GetTable", mock.Anything, "test_database",
"test_table").Return(testIcebergHiveTable1, nil).Once()
+
+ var tables []table.Identifier
+ for identifier, err := range NewCatalogWithClient(mockClient,
nil).ListTables(context.Background(), DatabaseIdentifier("test_database")) {
Review Comment:
Every other test in this file passes `iceberg.Properties{}` here; these new
ones pass `nil`. If `ApplyProperties` ever iterates the map without a nil guard
that's a setup panic masking the thing under test. I'd pass
`iceberg.Properties{}` to stay consistent.
##########
catalog/hive/hive_test.go:
##########
@@ -280,6 +280,39 @@ func TestHiveListTablesEmpty(t *testing.T) {
mockClient.AssertExpectations(t)
}
+func TestHiveListTablesHandlesObjectLoadingErrors(t *testing.T) {
+ t.Run("skips concurrently deleted tables", func(t *testing.T) {
+ mockClient := &mockHiveClient{}
+ mockClient.On("GetTables", mock.Anything, "test_database",
"*").Return([]string{"deleted", "test_table"}, nil).Once()
+ mockClient.On("GetTable", mock.Anything, "test_database",
"deleted").Return(nil, errNoSuchObject).Once()
+ mockClient.On("GetTable", mock.Anything, "test_database",
"test_table").Return(testIcebergHiveTable1, nil).Once()
+
+ var tables []table.Identifier
+ for identifier, err := range NewCatalogWithClient(mockClient,
nil).ListTables(context.Background(), DatabaseIdentifier("test_database")) {
+ require.NoError(t, err)
+ tables = append(tables, identifier)
+ }
+ require.Equal(t,
[]table.Identifier{TableIdentifier("test_database", "test_table")}, tables)
+ mockClient.AssertExpectations(t)
+ })
+
+ t.Run("propagates operational failures", func(t *testing.T) {
+ loadErr := errors.New("metastore unavailable")
+ mockClient := &mockHiveClient{}
+ mockClient.On("GetTables", mock.Anything, "test_database",
"*").Return([]string{"broken", "test_table"}, nil).Once()
+ mockClient.On("GetTable", mock.Anything, "test_database",
"broken").Return(nil, loadErr).Once()
+
+ var gotErr error
+ for _, err := range NewCatalogWithClient(mockClient,
nil).ListTables(context.Background(), DatabaseIdentifier("test_database")) {
+ gotErr = err
Review Comment:
This loop keeps going after the error and only holds the last one, so it
leans on the iterator stopping itself rather than checking that it does. I'd
break on the first error like a real consumer would, and while we're unpacking
both values, pin that the identifier is nil on the error yield:
```go
for ident, err := range NewCatalogWithClient(...).ListTables(...) {
if err != nil {
require.Nil(t, ident)
gotErr = err
break
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
