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 2020/12/18 05:50:27 UTC
[GitHub] [iceberg] zhangjun0x01 opened a new pull request #1958: Flink : remove static keyword from FlinkCatalog method
zhangjun0x01 opened a new pull request #1958:
URL: https://github.com/apache/iceberg/pull/1958
There are a few static methods in FlinkCatalog , but I found that there seems to be no code to call these methods directly through the class, such as `FlinkCatalog.validateFlinkTable`, so I think these `static` keywords should be removed
----------------------------------------------------------------
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 edited a comment on pull request #1958: Flink: remove static keyword from FlinkCatalog method
Posted by GitBox <gi...@apache.org>.
rdblue edited a comment on pull request #1958:
URL: https://github.com/apache/iceberg/pull/1958#issuecomment-748236369
@zhangjun0x01, I think `static` is a good practice because static methods don't have access to any instance state. That makes it easier to understand and reason about what the method is doing because it can only operate on its inputs. I think that is helpful for cases like `commitChanges` here: it is clear that the commit affects the `Table` passed in and does not alter any state in the catalog instance.
A similar situation is whether to use `static` for inner classes. I think it is easier to see in that case that keeping state separate from inner classes is a good practice when the inner class doesn't need to access state from the containing class. To me, static methods should be preferred for the same reason.
----------------------------------------------------------------
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] zhangjun0x01 closed pull request #1958: Flink: remove static keyword from FlinkCatalog method
Posted by GitBox <gi...@apache.org>.
zhangjun0x01 closed pull request #1958:
URL: https://github.com/apache/iceberg/pull/1958
----------------------------------------------------------------
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] zhangjun0x01 commented on pull request #1958: Flink: remove static keyword from FlinkCatalog method
Posted by GitBox <gi...@apache.org>.
zhangjun0x01 commented on pull request #1958:
URL: https://github.com/apache/iceberg/pull/1958#issuecomment-748705853
@rdblue thanks for your explanation,I close this pr
----------------------------------------------------------------
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 edited a comment on pull request #1958: Flink: remove static keyword from FlinkCatalog method
Posted by GitBox <gi...@apache.org>.
rdblue edited a comment on pull request #1958:
URL: https://github.com/apache/iceberg/pull/1958#issuecomment-748236369
@zhangjun0x01, I think `static` is a good practice because static methods don't have access to any instance state. That makes it easier to understand and reason about what the method is doing because it can only operate on its inputs. I think that is helpful for cases like `commitChanges` here: it is clear that the commit affects the `Table` passed in and not the catalog itself.
A similar situation is whether to use `static` for inner classes. I think it is easier to see in that case that keeping state separate from inner classes is a good practice when the inner class doesn't need to access state from the containing class. To me, static methods should be preferred for the same reason.
----------------------------------------------------------------
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 #1958: Flink: remove static keyword from FlinkCatalog method
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1958:
URL: https://github.com/apache/iceberg/pull/1958#issuecomment-748236369
@zhangjun0x01, I think `static` is a good practice because static methods also don't have access to any instance state. That makes it easier to understand and reason about what the method is doing because it can only operate on its inputs. I think that is helpful for cases like `commitChanges` here: it is clear that the commit affects the `Table` passed in and not the catalog itself.
A similar situation is whether to use `static` for inner classes. I think it is easier to see in that case that keeping state separate from inner classes is a good practice when the inner class doesn't need to access state from the containing class. To me, static methods should be preferred for the same reason.
----------------------------------------------------------------
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