You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/08/28 16:43:11 UTC
[GitHub] [hadoop-ozone] smengcl opened a new pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
smengcl opened a new pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363
This PR is derived from https://github.com/apache/hadoop-ozone/pull/1088 as the previous one is split into multiple jiras and needs rebase. I'm hoping opening up a brand new PR for the original jira would make things simpler.
## What changes were proposed in this pull request?
Use ClientProtocol directly in Adapter and FS.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3805
## How was this patch tested?
Existing tests should pass.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-687489498
> Thanks for the separation @smengcl, I think it's easier to discuss.
>
> I am not sure it's the same patch what I already commented or not. I had a comment in the previous PR, where the discussion is stopped:
>
> > > I'm in favor of A. I'll attempt to remove the usage of OzoneClientAdapter in OFS altogether then.
>
> > I am fine with that approach but let me add some comments to the latest patch.
>
> > The naming of BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl is misleading. Usually the Impl postfix is used when the class implemented a well known interface. There is no such interface here. (It's more like the delegation design pattern not an implementation)
>
> > As a test: Can you please explain what are the differences between the two classes and the responsibilities?
>
> > If not, we don't need two classes. Just remove the Impl and remove the dedicated methods and directly call the proxy from the original methods of BasicRootedOzoneFileSystem.
>
> > Wouldn't it be more simple?
>
> This patch seems to use `BasicRootedOzoneClientAdapterImpl` which is not an `Impl` (not an interface). Do you need a client adapter here (in the old code we need an interface and an implementation for classpath separation but here this separation is removed.)? If yes, do you need `Impl` in the name (it doesn't implement anything)?
I'm renaming `BasicRootedOzoneClientAdapterImpl` to `BasicRootedOzoneFileSystemHelper`. This way the name should make more 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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] github-actions[bot] commented on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-716905696
Thank you very much for the patch. I am closing this PR __temporarily__ as there was no activity recently and it is waiting for response from its author.
It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.
It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.
If you need ANY help to finish this PR, please [contact the community](https://github.com/apache/hadoop-ozone#contact) on the mailing list or the slack channel."
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] github-actions[bot] closed pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl edited a comment on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl edited a comment on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-687489498
> Thanks for the separation @smengcl, I think it's easier to discuss.
>
> I am not sure it's the same patch what I already commented or not. I had a comment in the previous PR, where the discussion is stopped:
>
> > > I'm in favor of A. I'll attempt to remove the usage of OzoneClientAdapter in OFS altogether then.
>
> > I am fine with that approach but let me add some comments to the latest patch.
>
> > The naming of BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl is misleading. Usually the Impl postfix is used when the class implemented a well known interface. There is no such interface here. (It's more like the delegation design pattern not an implementation)
>
> > As a test: Can you please explain what are the differences between the two classes and the responsibilities?
>
> > If not, we don't need two classes. Just remove the Impl and remove the dedicated methods and directly call the proxy from the original methods of BasicRootedOzoneFileSystem.
>
> > Wouldn't it be more simple?
>
> This patch seems to use `BasicRootedOzoneClientAdapterImpl` which is not an `Impl` (not an interface). Do you need a client adapter here (in the old code we need an interface and an implementation for classpath separation but here this separation is removed.)? If yes, do you need `Impl` in the name (it doesn't implement anything)?
I'm renaming `BasicRootedOzoneClientAdapterImpl` to `BasicRootedOzoneFileSystemHelper`. This way the name should make more sense?
But one uncertainly is that there is one class extending `BasicRootedOzoneClientAdapterImpl`:
```java
public class RootedOzoneClientAdapterImpl
extends BasicRootedOzoneClientAdapterImpl {
```
The class is dealing with `OzoneFSStorageStatistics`, but not used anywhere. The o3fs counterpart is `OzoneClientAdapterImpl`, which is used in `OzoneFileSystem`.
I am not sure how
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek edited a comment on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
elek edited a comment on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-694144300
> I'm thinking of renaming BasicRootedOzoneClientAdapterImpl to BasicRootedOzoneFileSystemHelper. This way the class name should make more sense?
Definitely better, IMHO. It's not clear why we need to move out some functions to a helper class (`"Can you please explain what are the differences between the two classes and the responsibilities?"`), but I can live with it, just to merge the patch earlier (not a big deal).
> The class is dealing with OzoneFSStorageStatistics, but not used anywhere. The o3fs counterpart is OzoneClientAdapterImpl, which is used in OzoneFileSystem.
This is only about the statistics. Independent to what type of classes do you have:
You need a `OzoneFSStorageStatistics storageStatistics` which is updated for each of the operations, but only for Hadoop3 (!!!).
For the old-school `o3fs` it's done by subclasses:
* Default implementation is hadoop2 compatibility as `incdementCounter` does nothing
* non-Basic implementation increments a real counter.
As far as I see we already have this logic for `ofs` as the imlementation of `RootedOzoneFileSystem` in `hadoop2` and `hadoop3` projects are different. (Later one update the statistics.)
If your helper class can be used from both project without problem, you don't need to create two helper classses for the two use-cases.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl edited a comment on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl edited a comment on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-687489498
> Thanks for the separation @smengcl, I think it's easier to discuss.
>
> I am not sure it's the same patch what I already commented or not. I had a comment in the previous PR, where the discussion is stopped:
>
> > > I'm in favor of A. I'll attempt to remove the usage of OzoneClientAdapter in OFS altogether then.
>
> > I am fine with that approach but let me add some comments to the latest patch.
>
> > The naming of BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl is misleading. Usually the Impl postfix is used when the class implemented a well known interface. There is no such interface here. (It's more like the delegation design pattern not an implementation)
>
> > As a test: Can you please explain what are the differences between the two classes and the responsibilities?
>
> > If not, we don't need two classes. Just remove the Impl and remove the dedicated methods and directly call the proxy from the original methods of BasicRootedOzoneFileSystem.
>
> > Wouldn't it be more simple?
>
> This patch seems to use `BasicRootedOzoneClientAdapterImpl` which is not an `Impl` (not an interface). Do you need a client adapter here (in the old code we need an interface and an implementation for classpath separation but here this separation is removed.)? If yes, do you need `Impl` in the name (it doesn't implement anything)?
I'm thinking of renaming `BasicRootedOzoneClientAdapterImpl` to `BasicRootedOzoneFileSystemHelper`. This way the class name should make more sense?
But one uncertainly is that there is one class extending `BasicRootedOzoneClientAdapterImpl`:
```java
public class RootedOzoneClientAdapterImpl
extends BasicRootedOzoneClientAdapterImpl {
```
The class is dealing with `OzoneFSStorageStatistics`, but not used anywhere. The o3fs counterpart is `OzoneClientAdapterImpl`, which is used in `OzoneFileSystem`.
I'm not sure of the implication of the renaming. What do you think?
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-684914342
Thanks for the separation @smengcl, I think it's easier to discuss.
I am not sure it's the same patch what I already commented or not. I had a comment in the previous PR, where the discussion is stopped:
>> I'm in favor of A. I'll attempt to remove the usage of OzoneClientAdapter in OFS altogether then.
> I am fine with that approach but let me add some comments to the latest patch.
> The naming of BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl is misleading. Usually the Impl postfix is used when the class implemented a well known interface. There is no such interface here. (It's more like the delegation design pattern not an implementation)
> As a test: Can you please explain what are the differences between the two classes and the responsibilities?
> If not, we don't need two classes. Just remove the Impl and remove the dedicated methods and directly call the proxy from the original methods of BasicRootedOzoneFileSystem.
> Wouldn't it be more simple?
This patch seems to use `BasicRootedOzoneClientAdapterImpl` which is not an `Impl` (not an interface). Do you need a client adapter here (in the old code we need an interface and an implementation for classpath separation but here this separation is removed.)? If yes, do you need `Impl` in the name (it doesn't implement anything)?
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-703644522
/pending
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl edited a comment on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl edited a comment on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-687489498
> Thanks for the separation @smengcl, I think it's easier to discuss.
>
> I am not sure it's the same patch what I already commented or not. I had a comment in the previous PR, where the discussion is stopped:
>
> > > I'm in favor of A. I'll attempt to remove the usage of OzoneClientAdapter in OFS altogether then.
>
> > I am fine with that approach but let me add some comments to the latest patch.
>
> > The naming of BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl is misleading. Usually the Impl postfix is used when the class implemented a well known interface. There is no such interface here. (It's more like the delegation design pattern not an implementation)
>
> > As a test: Can you please explain what are the differences between the two classes and the responsibilities?
>
> > If not, we don't need two classes. Just remove the Impl and remove the dedicated methods and directly call the proxy from the original methods of BasicRootedOzoneFileSystem.
>
> > Wouldn't it be more simple?
>
> This patch seems to use `BasicRootedOzoneClientAdapterImpl` which is not an `Impl` (not an interface). Do you need a client adapter here (in the old code we need an interface and an implementation for classpath separation but here this separation is removed.)? If yes, do you need `Impl` in the name (it doesn't implement anything)?
I'm renaming `BasicRootedOzoneClientAdapterImpl` to `BasicRootedOzoneFileSystemHelper`. This way the name should make more sense?
But one uncertainly is that there is one class extending `BasicRootedOzoneClientAdapterImpl`:
```java
public class RootedOzoneClientAdapterImpl
extends BasicRootedOzoneClientAdapterImpl {
```
The class is dealing with `OzoneFSStorageStatistics`, but not used anywhere. The o3fs counterpart is `OzoneClientAdapterImpl`, which is used in `OzoneFileSystem`.
I'm not sure of the implication of the renaming.
What do you think?
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-694144300
> I'm thinking of renaming BasicRootedOzoneClientAdapterImpl to BasicRootedOzoneFileSystemHelper. This way the class name should make more sense?
Definitely better, IMHO. It's not clear why we need to move out some functions to a helper class (`"Can you please explain what are the differences between the two classes and the responsibilities?"`), but I can live with it, just to merge the patch earlier.
> The class is dealing with OzoneFSStorageStatistics, but not used anywhere. The o3fs counterpart is OzoneClientAdapterImpl, which is used in OzoneFileSystem.
This is only about the statistics. Independent to what type of classes do you have:
You need a `OzoneFSStorageStatistics storageStatistics` which is updated for each of the operations, but only for Hadoop3 (!!!).
For the old-school `o3fs` it's done by subclasses:
* Default implementation is hadoop2 compatibility as `incdementCounter` does nothing
* non-Basic implementation increments a real counter.
As far as I see we already have this logic for `ofs` as the imlementation of `RootedOzoneFileSystem` in `hadoop2` and `hadoop3` projects are different. (Later one update the statistics.)
If your helper class can be used from both project without problem, you don't need to create two helper classses for the two use-cases.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1363: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1363:
URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-683168570
Note the previous 5 commits are directly from the previous PR. The last commit 37a441426a013a84ec1abcaf42d84e2196f61c13 is to address the new `getTrashRoots` impl.
I wanted to use proxy directly, only to find that I would need to build what ObjectStore has already done for me when listing volumes. So I kept ObjectStore as class global variable.
----------------------------------------------------------------
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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org