You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/09/13 19:27:04 UTC
[GitHub] [iceberg] xiaoxuandev opened a new pull request, #5756: AWS: Preload S3 client in GlueCatalog
xiaoxuandev opened a new pull request, #5756:
URL: https://github.com/apache/iceberg/pull/5756
Preload S3 client in GlueCatalog when glueLakeFormationEnabled
--
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
[GitHub] [iceberg] xiaoxuandev commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog
Posted by GitBox <gi...@apache.org>.
xiaoxuandev commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r970057765
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -210,18 +211,30 @@ void initialize(
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
if (catalogProperties != null) {
- Map<String, String> tableSpecificCatalogProperties =
- ImmutableMap.<String, String>builder()
- .putAll(catalogProperties)
- .put(
- AwsProperties.LAKE_FORMATION_DB_NAME,
- IcebergToGlueConverter.getDatabaseName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .put(
- AwsProperties.LAKE_FORMATION_TABLE_NAME,
- IcebergToGlueConverter.getTableName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .build();
+ Map<String, String> tableSpecificCatalogProperties = Maps.newHashMap(catalogProperties);
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_DB_NAME,
+ IcebergToGlueConverter.getDatabaseName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_TABLE_NAME,
+ IcebergToGlueConverter.getTableName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+
+ if (awsProperties.glueLakeFormationEnabled()) {
+ try {
+ glue.getTable(
Review Comment:
This check is needed for Create Table scenario, where we can not preload S3 client since table has not been created yet.
--
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
[GitHub] [iceberg] jackye1995 merged pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
jackye1995 merged PR #5756:
URL: https://github.com/apache/iceberg/pull/5756
--
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
[GitHub] [iceberg] xiaoxuandev commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
xiaoxuandev commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r970089028
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -210,18 +211,30 @@ void initialize(
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
if (catalogProperties != null) {
- Map<String, String> tableSpecificCatalogProperties =
- ImmutableMap.<String, String>builder()
- .putAll(catalogProperties)
- .put(
- AwsProperties.LAKE_FORMATION_DB_NAME,
- IcebergToGlueConverter.getDatabaseName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .put(
- AwsProperties.LAKE_FORMATION_TABLE_NAME,
- IcebergToGlueConverter.getTableName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .build();
+ Map<String, String> tableSpecificCatalogProperties = Maps.newHashMap(catalogProperties);
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_DB_NAME,
+ IcebergToGlueConverter.getDatabaseName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_TABLE_NAME,
+ IcebergToGlueConverter.getTableName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+
+ if (awsProperties.glueLakeFormationEnabled()) {
+ try {
+ glue.getTable(
Review Comment:
Makes sense, fixed, thanks!
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r970261854
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -67,7 +70,9 @@ class GlueTableOperations extends BaseMetastoreTableOperations {
private final String tableName;
private final String fullTableName;
private final String commitLockEntityId;
- private final FileIO fileIO;
+ private FileIO fileIO;
+ private final Map<String, String> tableSpecificCatalogProperties;
Review Comment:
nit: final variables before non-final
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r971067069
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -198,6 +208,17 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
}
}
+ public static FileIO initializeFileIO(Map<String, String> properties, Object hadoopConf) {
Review Comment:
why not keep the old one in `GlueCatalog` and change that to static? That avoids these line changes and deletes in the other place. Also prefer to be protected instead of public
--
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
[GitHub] [iceberg] jackye1995 commented on pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#issuecomment-1247262752
Thanks for review @amogh-jahagirdar and thanks for the fix @xiaoxuandev , given this is a very specific AWS LakeFormation fix I will go ahead and merge it instead of waiting for more reviews. Thanks!
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r970262051
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -67,7 +70,9 @@ class GlueTableOperations extends BaseMetastoreTableOperations {
private final String tableName;
private final String fullTableName;
private final String commitLockEntityId;
- private final FileIO fileIO;
+ private FileIO fileIO;
+ private final Map<String, String> tableSpecificCatalogProperties;
+ private Object hadoopConf;
Review Comment:
we can make this final by setting it to null in other constructors
--
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
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r971161799
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -83,7 +88,8 @@ class GlueTableOperations extends BaseMetastoreTableOperations {
LockManager lockManager,
String catalogName,
AwsProperties awsProperties,
- FileIO fileIO,
+ Map<String, String> tableSpecificCatalogProperties,
Review Comment:
Can this just be called tableCatalogProperties?
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r970020804
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -210,18 +211,30 @@ void initialize(
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
if (catalogProperties != null) {
- Map<String, String> tableSpecificCatalogProperties =
- ImmutableMap.<String, String>builder()
- .putAll(catalogProperties)
- .put(
- AwsProperties.LAKE_FORMATION_DB_NAME,
- IcebergToGlueConverter.getDatabaseName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .put(
- AwsProperties.LAKE_FORMATION_TABLE_NAME,
- IcebergToGlueConverter.getTableName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .build();
+ Map<String, String> tableSpecificCatalogProperties = Maps.newHashMap(catalogProperties);
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_DB_NAME,
+ IcebergToGlueConverter.getDatabaseName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_TABLE_NAME,
+ IcebergToGlueConverter.getTableName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+
+ if (awsProperties.glueLakeFormationEnabled()) {
+ try {
+ glue.getTable(
Review Comment:
I think we don't need to have this check anymore? If it's LF enabled, just always prefetch
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r970079165
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -210,18 +211,30 @@ void initialize(
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
if (catalogProperties != null) {
- Map<String, String> tableSpecificCatalogProperties =
- ImmutableMap.<String, String>builder()
- .putAll(catalogProperties)
- .put(
- AwsProperties.LAKE_FORMATION_DB_NAME,
- IcebergToGlueConverter.getDatabaseName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .put(
- AwsProperties.LAKE_FORMATION_TABLE_NAME,
- IcebergToGlueConverter.getTableName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .build();
+ Map<String, String> tableSpecificCatalogProperties = Maps.newHashMap(catalogProperties);
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_DB_NAME,
+ IcebergToGlueConverter.getDatabaseName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_TABLE_NAME,
+ IcebergToGlueConverter.getTableName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+
+ if (awsProperties.glueLakeFormationEnabled()) {
+ try {
+ glue.getTable(
Review Comment:
and I don't think we should log a warning for that, because it is expected behavior, maybe debug is enough
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r970091499
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -210,18 +210,30 @@ void initialize(
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
if (catalogProperties != null) {
- Map<String, String> tableSpecificCatalogProperties =
- ImmutableMap.<String, String>builder()
- .putAll(catalogProperties)
- .put(
- AwsProperties.LAKE_FORMATION_DB_NAME,
- IcebergToGlueConverter.getDatabaseName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .put(
- AwsProperties.LAKE_FORMATION_TABLE_NAME,
- IcebergToGlueConverter.getTableName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .build();
+ Map<String, String> tableSpecificCatalogProperties = Maps.newHashMap(catalogProperties);
Review Comment:
I think we still prefer to use immutable map here to avoid any accidental change. We can keep the builder instead of building it on L224, and built it after deciding if s3 prefetch is needed
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r970261742
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -198,6 +208,17 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
}
}
+ private FileIO initializeFileIO(Map<String, String> properties) {
Review Comment:
I think we need to make initializeFileIO static method to be used between this class and GlueCatalog.
--
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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r970077968
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -210,18 +211,30 @@ void initialize(
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
if (catalogProperties != null) {
- Map<String, String> tableSpecificCatalogProperties =
- ImmutableMap.<String, String>builder()
- .putAll(catalogProperties)
- .put(
- AwsProperties.LAKE_FORMATION_DB_NAME,
- IcebergToGlueConverter.getDatabaseName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .put(
- AwsProperties.LAKE_FORMATION_TABLE_NAME,
- IcebergToGlueConverter.getTableName(
- tableIdentifier, awsProperties.glueCatalogSkipNameValidation()))
- .build();
+ Map<String, String> tableSpecificCatalogProperties = Maps.newHashMap(catalogProperties);
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_DB_NAME,
+ IcebergToGlueConverter.getDatabaseName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+ tableSpecificCatalogProperties.put(
+ AwsProperties.LAKE_FORMATION_TABLE_NAME,
+ IcebergToGlueConverter.getTableName(
+ tableIdentifier, awsProperties.glueCatalogSkipNameValidation()));
+
+ if (awsProperties.glueLakeFormationEnabled()) {
+ try {
+ glue.getTable(
Review Comment:
in that case can we only check for not found exception instead of all the glue exceptions?
--
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
[GitHub] [iceberg] xiaoxuandev commented on a diff in pull request #5756: AWS: Preload S3 client in GlueCatalog For LakeFormation enabled tables
Posted by GitBox <gi...@apache.org>.
xiaoxuandev commented on code in PR #5756:
URL: https://github.com/apache/iceberg/pull/5756#discussion_r971077719
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -198,6 +208,17 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
}
}
+ public static FileIO initializeFileIO(Map<String, String> properties, Object hadoopConf) {
Review Comment:
because we already reference GlueTableOperations(calling its constructor) from GlueCatalog, this way we still keep the dependency in only one direction.
--
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