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/01/11 14:38:04 UTC
[GitHub] [iceberg] hililiwei opened a new pull request #3879: Core: Remove duplicate Conf() method.
hililiwei opened a new pull request #3879:
URL: https://github.com/apache/iceberg/pull/3879
conf() and getConf() are duplicates.
getConfig():
https://github.com/apache/iceberg/blob/f28ce8113474889c7d1ec40a1bb94a5ca6d82c8c/core/src/main/java/org/apache/iceberg/hadoop/HadoopFileIO.java#L84-L86
conf():
https://github.com/apache/iceberg/blob/f28ce8113474889c7d1ec40a1bb94a5ca6d82c8c/core/src/main/java/org/apache/iceberg/hadoop/HadoopFileIO.java#L53-L55
--
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] pvary commented on pull request #3879: Core: Remove duplicate Conf() method.
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #3879:
URL: https://github.com/apache/iceberg/pull/3879#issuecomment-1011892072
Since it is `Hadoop`FileIo, I think it is "natural" to expect it to implement `org.apache.hadoop.conf.Configurable`. That would mean that we would want to keep `getConf()`. OTOH in Iceberg `conf()` is the way to access attributes.
I would vote for keeping both, but a comment might be useful to encourage the usage of the `conf()` method.
--
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] rdblue commented on pull request #3879: Core: Remove duplicate Conf() method.
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3879:
URL: https://github.com/apache/iceberg/pull/3879#issuecomment-1015590921
I agree with @pvary. Let's skip this change. I don't think it is bad to have both methods.
--
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] kbendick edited a comment on pull request #3879: Core: Remove duplicate Conf() method.
Posted by GitBox <gi...@apache.org>.
kbendick edited a comment on pull request #3879:
URL: https://github.com/apache/iceberg/pull/3879#issuecomment-1011872346
Hi @hililiwei. Thanks for looking into this.
My thoughts are:
1) We tend to prefer method calls that don't use `get` in the name (or `set`). Either some other verb is used to be more informative (examples like `find`, etc) or we just don't use `get` since it likely provides little meaning if there isn't a more specific verb to use. That's probably why we have the two methods. So if we do remove one, it would be more in line with the project style to keep `.conf()` and remove `.getConf()`.
2) This is a rather old API. My opinion is we can't necessarily be sure if people are depending on this API or not. I'm interested in hearing other people's opinions on this, but I think it might be safer to keep both and mark `getConf` (or at least one of them) as deprecated.
Please don't spend the time updating the PR to use `conf()` instead of `getConf()` unless somebody else chimes in though so you don't make unnecessary updates 😄
--
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] hililiwei commented on pull request #3879: Core: Remove duplicate Conf() method.
Posted by GitBox <gi...@apache.org>.
hililiwei commented on pull request #3879:
URL: https://github.com/apache/iceberg/pull/3879#issuecomment-1011878742
@kbendick thanks for your comment, already got your gist.Let's wait for others' suggestions and then consider whether to make this change. 😄
--
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] kbendick commented on pull request #3879: Core: Remove duplicate Conf() method.
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3879:
URL: https://github.com/apache/iceberg/pull/3879#issuecomment-1011872346
Hi @hililiwei. Thanks for looking into this.
My thoughts are:
1) We tend to prefer method calls that don't use `get`. Either some other verb is used to be more informative (examples like `find`, etc) or we just don't use `get` since it has very little meaning. That's probably why we have the two methods. So if we do remove one, it would be more in line with the project style to keep `.conf()` and remove `.getConf()`.
2) This is a rather old API. My opinion is we can't necessarily be sure if people are depending on this API or not. I'm interested in hearing other people's opinions on this, but I think it might be safer to keep both and mark `getConf` (or at least one of them) as deprecated.
Please don't spend the time updating the PR to use `conf()` instead of `getConf()` unless somebody else chimes in though so you don't make unnecessary updates 😄
--
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] rdblue closed pull request #3879: Core: Remove duplicate Conf() method.
Posted by GitBox <gi...@apache.org>.
rdblue closed pull request #3879:
URL: https://github.com/apache/iceberg/pull/3879
--
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