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 2021/01/08 04:01:35 UTC
[GitHub] [iceberg] jackye1995 opened a new pull request #2050: AWS: consolidate config key names before release
jackye1995 opened a new pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050
As discussed in #1891 with @rdblue , shorten AWS config keys and change the following names:
- `gluecatalog.*` to `glue.*`
- `s3fileio.*` to `s3.*`
- `staging.dir` to `staging-dir`
- `lock.impl` to `lock-impl`
- `part-size` to `part-size-bytes`
@danielcweeks are you okay with these changes?
----------------------------------------------------------------
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.
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] danielcweeks commented on a change in pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050#discussion_r554076436
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -112,13 +112,13 @@
* switch from uploading using a single put object request to uploading using multipart upload
* (default: 1.5).
*/
- public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3fileio.multipart.threshold";
+ public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3.multipart.threshold";
Review comment:
wouldn't this be `s3.multipart-threshold`?
----------------------------------------------------------------
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.
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] danielcweeks merged pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
danielcweeks merged pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050
----------------------------------------------------------------
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.
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] danielcweeks commented on pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050#issuecomment-756875454
I'm fine with updating the property names (especially adding the units), but I'm not clear on what convention is being used here. Using `-` as a delimiter seems pretty inconsistent with most projects I'm aware of (not that most tend to be internally consistent anyway).
@rdblue, If there is a specific convention, we should probably document so reviewers can point out inconsistencies going forward.
----------------------------------------------------------------
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.
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] danielcweeks commented on a change in pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050#discussion_r554076436
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -112,13 +112,13 @@
* switch from uploading using a single put object request to uploading using multipart upload
* (default: 1.5).
*/
- public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3fileio.multipart.threshold";
+ public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3.multipart.threshold";
Review comment:
wouldn't this be `s3.multipart-threshold`?
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -112,13 +112,13 @@
* switch from uploading using a single put object request to uploading using multipart upload
* (default: 1.5).
*/
- public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3fileio.multipart.threshold";
+ public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3.multipart.threshold";
Review comment:
yup, that makes sense
----------------------------------------------------------------
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.
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] danielcweeks commented on a change in pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050#discussion_r554107821
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -112,13 +112,13 @@
* switch from uploading using a single put object request to uploading using multipart upload
* (default: 1.5).
*/
- public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3fileio.multipart.threshold";
+ public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3.multipart.threshold";
Review comment:
yup, that makes sense
----------------------------------------------------------------
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.
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] rdblue commented on pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050#issuecomment-756931684
@danielcweeks, the convention is to use `.` as a delimiter for grouping and `-` between multiple words of the same term.
For example, in `s3fileio.multipart.part.size` the `part` is not a group or namespace for properties, it is one word of "part size". Similarly, `bytes` is part of the thing being configured and we would not expect to see other properties nested within `size`.
Other projects use a convention where all words are separated by `.` regardless, but I think it is easier to understand the context hierarchy if things described by more than one word are kept together.
Also, `lock.impl` vs `lock-impl` is a fuzzy case since `lock` _is_ a group, but I think this should match the others, like `io-impl`.
----------------------------------------------------------------
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.
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 change in pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050#discussion_r554106569
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -112,13 +112,13 @@
* switch from uploading using a single put object request to uploading using multipart upload
* (default: 1.5).
*/
- public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3fileio.multipart.threshold";
+ public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3.multipart.threshold";
Review comment:
My understanding is that `s3.multipart.*` has all the multipart upload configs, that's why I kept it unchanged.
----------------------------------------------------------------
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.
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] danielcweeks merged pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
danielcweeks merged pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050
----------------------------------------------------------------
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.
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 change in pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050#discussion_r554106569
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -112,13 +112,13 @@
* switch from uploading using a single put object request to uploading using multipart upload
* (default: 1.5).
*/
- public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3fileio.multipart.threshold";
+ public static final String S3FILEIO_MULTIPART_THRESHOLD_FACTOR = "s3.multipart.threshold";
Review comment:
My understanding is that `s3.multipart.*` has all the multipart upload configs, that's why I kept it unchanged.
----------------------------------------------------------------
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.
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] rdblue commented on pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050#issuecomment-756931684
@danielcweeks, the convention is to use `.` as a delimiter for grouping and `-` between multiple words of the same term.
For example, in `s3fileio.multipart.part.size` the `part` is not a group or namespace for properties, it is one word of "part size". Similarly, `bytes` is part of the thing being configured and we would not expect to see other properties nested within `size`.
Other projects use a convention where all words are separated by `.` regardless, but I think it is easier to understand the context hierarchy if things described by more than one word are kept together.
Also, `lock.impl` vs `lock-impl` is a fuzzy case since `lock` _is_ a group, but I think this should match the others, like `io-impl`.
----------------------------------------------------------------
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.
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] danielcweeks commented on pull request #2050: AWS: consolidate config key names before release
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on pull request #2050:
URL: https://github.com/apache/iceberg/pull/2050#issuecomment-756875454
I'm fine with updating the property names (especially adding the units), but I'm not clear on what convention is being used here. Using `-` as a delimiter seems pretty inconsistent with most projects I'm aware of (not that most tend to be internally consistent anyway).
@rdblue, If there is a specific convention, we should probably document so reviewers can point out inconsistencies going forward.
----------------------------------------------------------------
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.
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