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/06/15 07:52:27 UTC
[GitHub] [hadoop-ozone] cxorm opened a new pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
cxorm opened a new pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075
## What changes were proposed in this pull request?
According the volume operations under
[`hadoop-ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume`](https://github.com/apache/hadoop-ozone/tree/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume)
we could get **write** operations as follows:
- Create Volume
- Delete Volume
- Update Volume
- Set Quota
- Set Owner
- Acl Operations
- Set Acl
- Add Acl
- Remove Acl
#### Proposed fix.
- Cleanup old write-path of volume in [`OzoneManger.java`](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java)
- After cleaning old write-path of volume in `OzoneManager.java`, we could also clean up the write-operation in [`VolumeManager.java`](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/VolumeManager.java)
- **note :** This part didn't clean the `createVolume()` because it is used by [`BenchMarkOMKeyAllocation.java`](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/genesis/BenchMarkOMKeyAllocation.java)
- Remove test corresponding to the cleanup.
- I think we could refactor the [`TestOmMetrics.java`](https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmMetrics.java) to HA-version. (This PR just removed volume-related test.)
- **Propose** creating jiras for fixing/updating `BenchMarkOMKeyAllocation.java` and `TestOmMetrics.java`
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3369
## How was this patch tested?
[Github action passed](https://github.com/cxorm/hadoop-ozone/runs/771451070)
----------------------------------------------------------------
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] cxorm commented on pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075#issuecomment-648723678
Hi @bharatviswa504,
Could you please take a look on this PR if you have a time : )
----------------------------------------------------------------
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] cxorm commented on a change in pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075#discussion_r467757851
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
##########
@@ -82,15 +85,19 @@
* already the owner.
* @throws IOException
*/
- boolean setOwner(String volume, String owner) throws IOException;
+ default boolean setOwner(String volume, String owner) throws IOException {
+ throw new NotImplementedException("setOwner is not implemented");
Review comment:
> In the long run I think we should split `OzoneManagerProtocol` interface into separate read and write interfaces. `OzoneManager` should only implement read interface, and `OzoneManagerProtocolClientSideTranslatorPB` should implement both.
Thanks @adoroszlai for the idea.
I'm fine with this proposal, and I would split the `OzoneManagerProtocol` interface into separate read and write interfaces in this patch. (Feel free to give me feedback or share your thoughts, 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.
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] cxorm closed pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
cxorm closed pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075
----------------------------------------------------------------
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] cxorm commented on a change in pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075#discussion_r467770706
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
##########
@@ -82,15 +85,19 @@
* already the owner.
* @throws IOException
*/
- boolean setOwner(String volume, String owner) throws IOException;
+ default boolean setOwner(String volume, String owner) throws IOException {
+ throw new NotImplementedException("setOwner is not implemented");
Review comment:
> In the long run I think we should split `OzoneManagerProtocol` interface into separate read and write interfaces. `OzoneManager` should only implement read interface, and `OzoneManagerProtocolClientSideTranslatorPB` should implement both.
[Edit Comment]
Thanks @adoroszlai for the idea.
I agree it, and I would share some thoughts about this work.
----------------------------------------------------------------
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] adoroszlai commented on a change in pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075#discussion_r450014240
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/VolumeManager.java
##########
@@ -35,25 +35,6 @@
void createVolume(OmVolumeArgs args)
Review comment:
I think it's safe to remove this. `BenchMarkOMKeyAllocation` does not work anyway:
```
$ bin/ozone genesis -b BenchMarkOMKeyAllocation
...
VOLUME_NOT_FOUND org.apache.hadoop.ozone.om.exceptions.OMException: Volume doesn't exist
at org.apache.hadoop.ozone.om.BucketManagerImpl.createBucket(BucketManagerImpl.java:130)
at org.apache.hadoop.ozone.genesis.BenchMarkOMKeyAllocation.setup(BenchMarkOMKeyAllocation.java:86)
at org.apache.hadoop.ozone.genesis.generated.BenchMarkOMKeyAllocation_keyCreation_jmhTest._jmh_tryInit_f_benchmarkomkeyallocation0_0(BenchMarkOMKeyAllocation_keyCreation_jmhTest.java:338)
at org.apache.hadoop.ozone.genesis.generated.BenchMarkOMKeyAllocation_keyCreation_jmhTest.keyCreation_Throughput(BenchMarkOMKeyAllocation_keyCreation_jmhTest.java:71)
```
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
##########
@@ -72,7 +73,9 @@
* @param args - Arguments to create Volume.
* @throws IOException
*/
- void createVolume(OmVolumeArgs args) throws IOException;
+ default void createVolume(OmVolumeArgs args) throws IOException {
+ throw new NotImplementedException("createVolume is not implemented");
Review comment:
`createVolume` is called directly from `BenchMarkOzoneManager`:
```
$ bin/ozone genesis -b BenchMarkOzoneManager
...
org.apache.commons.lang3.NotImplementedException: createVolume is not implemented
at org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol.createVolume(OzoneManagerProtocol.java:77)
at org.apache.hadoop.ozone.genesis.BenchMarkOzoneManager.initialize(BenchMarkOzoneManager.java:110)
at org.apache.hadoop.ozone.genesis.generated.BenchMarkOzoneManager_allocateBlockBenchMark_jmhTest._jmh_tryInit_f_benchmarkozonemanager0_0(BenchMarkOzoneManager_allocateBlockBenchMark_jmhTest.java:351)
at org.apache.hadoop.ozone.genesis.generated.BenchMarkOzoneManager_allocateBlockBenchMark_jmhTest.allocateBlockBenchMark_Throughput(BenchMarkOzoneManager_allocateBlockBenchMark_jmhTest.java:72)
```
`BenchMarkOzoneManager` no longer works even on `master` (same error as with `BenchMarkOMKeyAllocation`).
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
##########
@@ -82,15 +85,19 @@
* already the owner.
* @throws IOException
*/
- boolean setOwner(String volume, String owner) throws IOException;
+ default boolean setOwner(String volume, String owner) throws IOException {
+ throw new NotImplementedException("setOwner is not implemented");
Review comment:
Instead of changing the interface, I think it would be safer to throw exception only in `OzoneManager`'s "implementation". That way new implementors would be required to implement these methods or deliberately decide not to.
If you agree, then please also consider changing to `UnsupportedOperationException` (as
> [`NotImplementedException`](https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/NotImplementedException.html) represents the case where the author has yet to implement the logic at this point in the program
which would not be the case with `OzoneManager`.
In the long run I think we should split `OzoneManagerProtocol` interface into separate read and write interfaces. `OzoneManager` should only implement read interface, and `OzoneManagerProtocolClientSideTranslatorPB` should implement both.
----------------------------------------------------------------
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] cxorm commented on a change in pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075#discussion_r467757851
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
##########
@@ -82,15 +85,19 @@
* already the owner.
* @throws IOException
*/
- boolean setOwner(String volume, String owner) throws IOException;
+ default boolean setOwner(String volume, String owner) throws IOException {
+ throw new NotImplementedException("setOwner is not implemented");
Review comment:
> In the long run I think we should split `OzoneManagerProtocol` interface into separate read and write interfaces. `OzoneManager` should only implement read interface, and `OzoneManagerProtocolClientSideTranslatorPB` should implement both.
Thanks @adoroszlai for the thought.
I'm fine with this proposal, and I would split the `OzoneManagerProtocol` interface into separate read and write interfaces in this patch. (Feel free to give me feedback or share your thoughts, 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.
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] cxorm commented on a change in pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075#discussion_r467757851
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
##########
@@ -82,15 +85,19 @@
* already the owner.
* @throws IOException
*/
- boolean setOwner(String volume, String owner) throws IOException;
+ default boolean setOwner(String volume, String owner) throws IOException {
+ throw new NotImplementedException("setOwner is not implemented");
Review comment:
> In the long run I think we should split `OzoneManagerProtocol` interface into separate read and write interfaces. `OzoneManager` should only implement read interface, and `OzoneManagerProtocolClientSideTranslatorPB` should implement both.
Thanks @adoroszlai for the idea.
I'm fine with this proposal, and I would split the `OzoneManagerProtocol` interface into separate read and write interfaces in this patch. (Feel free to give me feedback or share your thoughts, 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.
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] cxorm commented on pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075#issuecomment-685191223
Thank you @adoroszlai for the advise.
Splitting `OzoneManagerProtocol` interface is huge work IMHO.
I think it is improper to change the interface after GA-release,
so I propose 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: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] cxorm commented on a change in pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075#discussion_r467770706
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
##########
@@ -82,15 +85,19 @@
* already the owner.
* @throws IOException
*/
- boolean setOwner(String volume, String owner) throws IOException;
+ default boolean setOwner(String volume, String owner) throws IOException {
+ throw new NotImplementedException("setOwner is not implemented");
Review comment:
> In the long run I think we should split `OzoneManagerProtocol` interface into separate read and write interfaces. `OzoneManager` should only implement read interface, and `OzoneManagerProtocolClientSideTranslatorPB` should implement both.
[Edit Comment]
Thanks @adoroszlai for the idea.
I would share some thoughts about this work.
----------------------------------------------------------------
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] cxorm commented on pull request #1075: HDDS-3369. Cleanup old write-path of volume in OM
Posted by GitBox <gi...@apache.org>.
cxorm commented on pull request #1075:
URL: https://github.com/apache/hadoop-ozone/pull/1075#issuecomment-662881106
Sorry for I didn't have computer for last two weeks :cry:
Looking on this PR again.
----------------------------------------------------------------
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