Implement pre-existing session support for dynamodb catalog by waifairer · Pull Request #104 · apache/iceberg-python

waifairer · 2023-10-25T19:17:21Z

Looks like Dynamo is lacking some properties which are documented for other AWS-centric catalog types.

Fokko · 2023-10-25T19:24:29Z

Thanks @waifairer for raising this! Could you also update the docs under mkdocs/docs/configuration.md?

waifairer · 2023-10-25T19:36:16Z

Done, thanks @Fokko

HonahX

Thanks @waifairer for the contribution! It looks good to me! Just have one comment about adding a unit test for this.

HonahX · 2023-10-26T00:37:02Z

+            aws_secret_access_key=properties.get("aws_secret_access_key"),
+            aws_session_token=properties.get("aws_session_token"),
+        )
+        self.dynamodb = session.client(DYNAMODB_CLIENT)


Shall we add a unit test for this update? Like this one in GlueCatalog's test:

iceberg-python/tests/catalog/test_glue.py

Lines 442 to 459 in d8bc9a9

@mock_glue

def test_passing_profile_name() -> None:

session_properties: Dict[str, Any] = {

"aws_access_key_id": "abc",

"aws_secret_access_key": "def",

"aws_session_token": "ghi",

"region_name": "eu-central-1",

"profile_name": "sandbox",

"botocore_session": None,

}

test_properties = {"type": "glue"}

test_properties.update(session_properties)

with mock.patch("boto3.Session") as mock_session:

test_catalog = GlueCatalog("glue", **test_properties)

mock_session.assert_called_with(**session_properties)

assert test_catalog.glue is mock_session().client()

Hey @HonahX , thanks for the suggestion. I added the unit test as you requested.

Thanks for adding the test!

Fokko · 2023-10-26T19:35:43Z

+catalog:
+  default:
+    type: dynamodb
+    table-name: iceberg


Just noticed that we're mixing case here 😭

oh yeah.. i mean the underscores are pretty typical aws syntax but iceberg has always used hyphens afaik

I think we have an inconsistency since the FileIO configurations use hyphens for s3 credentials:

iceberg-python/mkdocs/docs/configuration.md

Lines 66 to 67 in d8bc9a9

| s3.access-key-id | admin | Configure the static secret access key used to access the FileIO. |

| s3.secret-access-key | password | Configure the static session token used to access the FileIO. |

Also, what about adding a dynamo. prefix to these credential keys?

dynamodb.access-key-id dynamodb.secret-access-key ...

This could help clarify that these keys are used exclusively by the catalog, not the FileIO:

Just thinking ahead, if we're in favor of renaming these configurations, perhaps it's something we could roll out in a separate PR, especially considering updates to the GlueCatalog configurations that would follow.

I'd love to hear your thoughts on this.

@HonahX Definitely agreed with dynamo as a prefix.

As for hyphens vs underscores, AWS is really consistent about using underscores. I'm of the opinion that the AWS-based credentials should support both underscores and hyphens, will prefer hyphens if present, but fall back to the underscore usages if necessary. Documentation should only present the hyphenated case as an option. I believe that this strategy would lead to the least number of "head banging" debug sessions. However, I think a reasonable case could be made to remove underscore support instead of supporting it as a fallback.

@waifairer Yes, I totally agree. Let's do that in a separate PR.

HonahX · 2023-11-03T00:05:56Z

        except self.dynamodb.exceptions.InternalServerError as e:
            raise GenericDynamoDbError(e.message) from e
-
+        print(response["Table"]["TableStatus"])


Nit: could you please remove this print?

Not a nit at all, thanks for the catch

HonahX · 2023-11-03T00:10:36Z

+            aws_secret_access_key=properties.get("aws_secret_access_key"),
+            aws_session_token=properties.get("aws_session_token"),
+        )
+        self.dynamodb = session.client(DYNAMODB_CLIENT)


Thanks for adding the test!

HonahX

LGTM! Just adding some thoughts on the configuration naming

HonahX · 2023-11-03T06:40:57Z

+catalog:
+  default:
+    type: dynamodb
+    table-name: iceberg


I think we have an inconsistency since the FileIO configurations use hyphens for s3 credentials:

iceberg-python/mkdocs/docs/configuration.md

Lines 66 to 67 in d8bc9a9

| s3.access-key-id | admin | Configure the static secret access key used to access the FileIO. |

| s3.secret-access-key | password | Configure the static session token used to access the FileIO. |

Also, what about adding a dynamo. prefix to these credential keys?

dynamodb.access-key-id dynamodb.secret-access-key ...

This could help clarify that these keys are used exclusively by the catalog, not the FileIO:

Just thinking ahead, if we're in favor of renaming these configurations, perhaps it's something we could roll out in a separate PR, especially considering updates to the GlueCatalog configurations that would follow.

I'd love to hear your thoughts on this.

Fokko

Sorry for the long wait @waifairer LGTM 🙌 Thanks for working on this, and @HonahX thanks for the review

Fokko · 2024-01-18T10:55:56Z

+catalog:
+  default:
+    type: dynamodb
+    table-name: iceberg


@waifairer Yes, I totally agree. Let's do that in a separate PR.

Implement pre-existing session support for dynamodb catalog

668857d

Update documentation

74f1fdf

HonahX reviewed Oct 26, 2023

View reviewed changes

Fokko reviewed Oct 26, 2023

View reviewed changes

Add unit test

d44eb11

waifairer requested review from Fokko and HonahX November 2, 2023 20:08

EOF

6aca298

HonahX reviewed Nov 3, 2023

View reviewed changes

Remove print

2f7d1c6

HonahX approved these changes Nov 3, 2023

View reviewed changes

Fokko added this to the PyIceberg 0.6.0 release milestone Dec 13, 2023

Fokko approved these changes Jan 18, 2024

View reviewed changes

Fokko merged commit d796878 into apache:main Jan 18, 2024

	@mock_glue
	def test_passing_profile_name() -> None:
	session_properties: Dict[str, Any] = {
	"aws_access_key_id": "abc",
	"aws_secret_access_key": "def",
	"aws_session_token": "ghi",
	"region_name": "eu-central-1",
	"profile_name": "sandbox",
	"botocore_session": None,
	}
	test_properties = {"type": "glue"}
	test_properties.update(session_properties)

	with mock.patch("boto3.Session") as mock_session:
	test_catalog = GlueCatalog("glue", **test_properties)

	mock_session.assert_called_with(**session_properties)
	assert test_catalog.glue is mock_session().client()

	\| s3.access-key-id \| admin \| Configure the static secret access key used to access the FileIO. \|
	\| s3.secret-access-key \| password \| Configure the static session token used to access the FileIO. \|

Conversation

waifairer commented Oct 25, 2023

Uh oh!

Fokko commented Oct 25, 2023

Uh oh!

waifairer commented Oct 25, 2023

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HonahX Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HonahX Nov 3, 2023 •

edited

Loading