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 2021/09/10 02:25:44 UTC
[GitHub] [ozone] GeorgeJahad opened a new pull request #2629: HDDS-3371. Begin Cleanup old write-path of key in OM
GeorgeJahad opened a new pull request #2629:
URL: https://github.com/apache/ozone/pull/2629
## What changes were proposed in this pull request?
When the OM-HA code was added, the tests were not completely updated. Many of them still call the OzoneManagerProtocol "write-path" methods even though those methods have been superseded by the HA methods.
This PR is a small beginning to the fix. I didn't want to do too much before getting some feedback, so I just removed a single api call, OzoneManager.deleteKey(), and fixed the corresponding tests, by changing them to invoke the ozoneManagerClient version.
If there is a less hacky way of doing it, without using the client, please let me know. I couldn't find one. I tried creating an alternative, but it looked like a half-baked client, so I just used the real one.
Most of the tests here are fixed by just replacing the OM with the client. I timed all the ones I changed and didn't notice any running longer.
### Notes on TestOmMetrics.testKeyOps() test
The only test that was difficult to fix was the testKeyOps() test [here](https://github.com/apache/ozone/blob/79a9d39da7f33e71bc00183e280105562354cca4/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmMetrics.java#L243).
This test verifies that keyOps generate the correct number of metrics. I tried to write the new code so the numbers didn't change. But there is actually a flaw in the original test that causes it to undercount a few, (*NumKeyLookupFails*, and *NumKeyDeleteFails*). So you'll see those fixed.
The test depends on mocks of the KeyManager. But now that the OM no longer supports deleteKey(), those mocks won't work. So I had to replace most of them with actual working code; (because I couldn't find a way to mock the actual write path.)
The test is divided into two halves. The first half verifies normal metrics; the second half verifies failure metrics. I removed the mocks in the first half; those metrics are now generated by real code. The mocks in the second half are there to force failures/exceptions. I modified those to work with the new code.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3371
## How was this patch tested?
All effected tests were updated.
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#issuecomment-916606247
> @bharatviswa504 This is just the start of what you requested, but I didn't want to go too far without you confirming that my approach was right. So please take a look when you get a chance.
Thank You @GeorgeJahad for the work, over all approach LGTM. Few questions I have commented them inline
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] cxorm commented on a change in pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
cxorm commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r705887099
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2682,27 +2682,8 @@ public void renameKey(OmKeyArgs args, String toKeyName) throws IOException {
*/
@Override
public void deleteKey(OmKeyArgs args) throws IOException {
- Map<String, String> auditMap = args.toAuditMap();
- try {
- ResolvedBucket bucket = resolveBucketLink(args);
- args = bucket.update(args);
-
- if (isAclEnabled) {
- checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.DELETE,
- bucket.realVolume(), bucket.realBucket(), args.getKeyName());
- }
-
- metrics.incNumKeyDeletes();
- keyManager.deleteKey(args);
- AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.DELETE_KEY,
- auditMap));
- metrics.decNumKeys();
- } catch (Exception ex) {
- metrics.incNumKeyDeleteFails();
- AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.DELETE_KEY,
- auditMap, ex));
- throw ex;
- }
+ throw new UnsupportedOperationException("OzoneManager does not require " +
Review comment:
I'm +1 for removing these methods.
IMHO there are two ways to do it,
1. Change the `OzoneManagerProtocol`.
> It is a **huge** patch, and we would encounter many issues like backward-compatibility and API compatibility.
2. Deprecating `OzoneManager.java`.
> In this case, we should update those part of using `OzoneManager.java`. (Still a `huge` patch.)
These ways are too complicated to archive in a clear manner.
I ever used to the same throw-Exception manner and I think it doesn't solve the problem fully.
Feel free to correct me if you have any idea.
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r705872167
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2682,27 +2682,8 @@ public void renameKey(OmKeyArgs args, String toKeyName) throws IOException {
*/
@Override
public void deleteKey(OmKeyArgs args) throws IOException {
- Map<String, String> auditMap = args.toAuditMap();
- try {
- ResolvedBucket bucket = resolveBucketLink(args);
- args = bucket.update(args);
-
- if (isAclEnabled) {
- checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.DELETE,
- bucket.realVolume(), bucket.realBucket(), args.getKeyName());
- }
-
- metrics.incNumKeyDeletes();
- keyManager.deleteKey(args);
- AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.DELETE_KEY,
- auditMap));
- metrics.decNumKeys();
- } catch (Exception ex) {
- metrics.incNumKeyDeleteFails();
- AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.DELETE_KEY,
- auditMap, ex));
- throw ex;
- }
+ throw new UnsupportedOperationException("OzoneManager does not require " +
Review comment:
Just a question, instead of doing this way, is there a way we can remove these methods.
My question is coming from the context, as if any new methods added to OzoneManagerProtocol, still should add this UnsupportedOperationException?
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r705872712
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2682,27 +2682,8 @@ public void renameKey(OmKeyArgs args, String toKeyName) throws IOException {
*/
@Override
public void deleteKey(OmKeyArgs args) throws IOException {
- Map<String, String> auditMap = args.toAuditMap();
- try {
- ResolvedBucket bucket = resolveBucketLink(args);
- args = bucket.update(args);
-
- if (isAclEnabled) {
- checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.DELETE,
- bucket.realVolume(), bucket.realBucket(), args.getKeyName());
- }
-
- metrics.incNumKeyDeletes();
- keyManager.deleteKey(args);
- AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.DELETE_KEY,
- auditMap));
- metrics.decNumKeys();
- } catch (Exception ex) {
- metrics.incNumKeyDeleteFails();
- AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.DELETE_KEY,
- auditMap, ex));
- throw ex;
- }
+ throw new UnsupportedOperationException("OzoneManager does not require " +
Review comment:
If that is too much change, we can explore that in a new Jira. Lets get the work going in, as I see it is causing lot of confusion to OM developers
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r705872167
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2682,27 +2682,8 @@ public void renameKey(OmKeyArgs args, String toKeyName) throws IOException {
*/
@Override
public void deleteKey(OmKeyArgs args) throws IOException {
- Map<String, String> auditMap = args.toAuditMap();
- try {
- ResolvedBucket bucket = resolveBucketLink(args);
- args = bucket.update(args);
-
- if (isAclEnabled) {
- checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.DELETE,
- bucket.realVolume(), bucket.realBucket(), args.getKeyName());
- }
-
- metrics.incNumKeyDeletes();
- keyManager.deleteKey(args);
- AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.DELETE_KEY,
- auditMap));
- metrics.decNumKeys();
- } catch (Exception ex) {
- metrics.incNumKeyDeleteFails();
- AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.DELETE_KEY,
- auditMap, ex));
- throw ex;
- }
+ throw new UnsupportedOperationException("OzoneManager does not require " +
Review comment:
Just a question, instead of doing this way, is there a way we can remove these methods.
My question is coming from the context, as if any new write requests added to OzoneManagerProtocol, still we should add this UnsupportedOperationException?
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r710669274
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2682,27 +2682,8 @@ public void renameKey(OmKeyArgs args, String toKeyName) throws IOException {
*/
@Override
public void deleteKey(OmKeyArgs args) throws IOException {
- Map<String, String> auditMap = args.toAuditMap();
- try {
- ResolvedBucket bucket = resolveBucketLink(args);
- args = bucket.update(args);
-
- if (isAclEnabled) {
- checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.DELETE,
- bucket.realVolume(), bucket.realBucket(), args.getKeyName());
- }
-
- metrics.incNumKeyDeletes();
- keyManager.deleteKey(args);
- AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.DELETE_KEY,
- auditMap));
- metrics.decNumKeys();
- } catch (Exception ex) {
- metrics.incNumKeyDeleteFails();
- AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.DELETE_KEY,
- auditMap, ex));
- throw ex;
- }
+ throw new UnsupportedOperationException("OzoneManager does not require " +
Review comment:
> As of right now, I think they can be handled in separate PR's. And I think 1 is much higher priority, because as you've said, the code is causing so much confusion. What do you think, @bharatviswa504 ?
Yes lets remove old code first which is causing confusion.
> In other words, finish removing all the methods from OzoneManager.java first. And fix all the tests that that breaks. Then all the methods from the KeyManager.java and fix the tests that those break, and so on. Does that sound ok, @bharatviswa504 ?
Yes. I am +1.
I agree with all other responses also. Lets continue cleanup by class and make progress.
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad closed pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
GeorgeJahad closed pull request #2629:
URL: https://github.com/apache/ozone/pull/2629
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #2629: HDDS-3371. Begin Cleanup old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r705857855
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmMetrics.java
##########
@@ -276,33 +279,45 @@ public void testKeyOps() throws IOException {
assertCounter("NumTrashKeyLists", 1L, omMetrics);
assertCounter("NumKeys", 0L, omMetrics);
assertCounter("NumInitiateMultipartUploads", 1L, omMetrics);
+
-
- ozoneManager.openKey(keyArgs);
- ozoneManager.commitKey(keyArgs, 0);
- ozoneManager.openKey(keyArgs);
- ozoneManager.commitKey(keyArgs, 0);
- ozoneManager.openKey(keyArgs);
- ozoneManager.commitKey(keyArgs, 0);
- ozoneManager.deleteKey(keyArgs);
+ keyArgs = createKeyArgs();
Review comment:
In the previous version the mocks would allow the counters would increment, even though the same key was being written every time. To allow this to work now, I had to create new keyArgs, each time a new key was committed.
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on pull request #2629: HDDS-3371. Begin Cleanup old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#issuecomment-916590214
@bharatviswa504 This is just the start of what you requested, but I didn't want to go too far without you confirming that my approach was right. So please take a look when you get a chance.
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r710669274
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2682,27 +2682,8 @@ public void renameKey(OmKeyArgs args, String toKeyName) throws IOException {
*/
@Override
public void deleteKey(OmKeyArgs args) throws IOException {
- Map<String, String> auditMap = args.toAuditMap();
- try {
- ResolvedBucket bucket = resolveBucketLink(args);
- args = bucket.update(args);
-
- if (isAclEnabled) {
- checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.DELETE,
- bucket.realVolume(), bucket.realBucket(), args.getKeyName());
- }
-
- metrics.incNumKeyDeletes();
- keyManager.deleteKey(args);
- AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.DELETE_KEY,
- auditMap));
- metrics.decNumKeys();
- } catch (Exception ex) {
- metrics.incNumKeyDeleteFails();
- AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.DELETE_KEY,
- auditMap, ex));
- throw ex;
- }
+ throw new UnsupportedOperationException("OzoneManager does not require " +
Review comment:
> As of right now, I think they can be handled in separate PR's. And I think 1 is much higher priority, because as you've said, the code is causing so much confusion. What do you think, @bharatviswa504 ?
Yes lets remove old code first which is causing confusion.
> In other words, finish removing all the methods from OzoneManager.java first. And fix all the tests that that breaks. Then all the methods from the KeyManager.java and fix the tests that those break, and so on. Does that sound ok, @bharatviswa504 ?
Yes. I am +1.
I agree with all other responses also. Lets continue cleanup by class and make progress.
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #2629: HDDS-3371. Begin Cleanup old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r705857626
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmMetrics.java
##########
@@ -276,33 +279,45 @@ public void testKeyOps() throws IOException {
assertCounter("NumTrashKeyLists", 1L, omMetrics);
assertCounter("NumKeys", 0L, omMetrics);
assertCounter("NumInitiateMultipartUploads", 1L, omMetrics);
+
-
- ozoneManager.openKey(keyArgs);
- ozoneManager.commitKey(keyArgs, 0);
- ozoneManager.openKey(keyArgs);
- ozoneManager.commitKey(keyArgs, 0);
- ozoneManager.openKey(keyArgs);
- ozoneManager.commitKey(keyArgs, 0);
- ozoneManager.deleteKey(keyArgs);
+ keyArgs = createKeyArgs();
+ OpenKeySession keySession = writeClient.openKey(keyArgs);
+ writeClient.commitKey(keyArgs, keySession.getId());
+ keyArgs = createKeyArgs();
+ keySession = writeClient.openKey(keyArgs);
+ writeClient.commitKey(keyArgs, keySession.getId());
+ keyArgs = createKeyArgs();
+ keySession = writeClient.openKey(keyArgs);
+ writeClient.commitKey(keyArgs, keySession.getId());
+ writeClient.deleteKey(keyArgs);
omMetrics = getMetrics("OMMetrics");
assertCounter("NumKeys", 2L, omMetrics);
- // inject exception to test for Failure Metrics
- Mockito.doThrow(exception).when(mockKm).openKey(any());
- Mockito.doThrow(exception).when(mockKm).deleteKey(any());
+ // inject exception to test for Failure Metrics on the read path
Mockito.doThrow(exception).when(mockKm).lookupKey(any(), any());
Mockito.doThrow(exception).when(mockKm).listKeys(
any(), any(), any(), any(), anyInt());
Mockito.doThrow(exception).when(mockKm).listTrash(
any(), any(), any(), any(), anyInt());
- Mockito.doThrow(exception).when(mockKm).commitKey(any(), anyLong());
- Mockito.doThrow(exception).when(mockKm).initiateMultipartUpload(any());
-
HddsWhiteboxTestUtils.setInternalState(
ozoneManager, "keyManager", mockKm);
+
+ // inject exception to test for Failure Metrics on the write path
+ OMMetadataManager metadataManager = (OMMetadataManager)
+ HddsWhiteboxTestUtils.getInternalState(ozoneManager, "metadataManager");
+ OMMetadataManager mockMm = Mockito.spy(metadataManager);
+ Table<String, OmBucketInfo> bucketTable = (Table<String, OmBucketInfo>)
+ HddsWhiteboxTestUtils.getInternalState(metadataManager, "bucketTable");
+ Table<String, OmBucketInfo> mockBTable = Mockito.spy(bucketTable);
+ Mockito.doThrow(exception).when(mockBTable).isExist(any());
+ Mockito.doReturn(mockBTable).when(mockMm).getBucketTable();
Review comment:
Spotbugs reported this about this line so I added an exemption:
```
M D RV: Return value of OMMetadataManager.getBucketTable() ignored, but method has no side effect At TestOmMetrics.java:[line 316]"
```
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r705873006
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2682,27 +2682,8 @@ public void renameKey(OmKeyArgs args, String toKeyName) throws IOException {
*/
@Override
public void deleteKey(OmKeyArgs args) throws IOException {
- Map<String, String> auditMap = args.toAuditMap();
- try {
- ResolvedBucket bucket = resolveBucketLink(args);
- args = bucket.update(args);
-
- if (isAclEnabled) {
- checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.DELETE,
- bucket.realVolume(), bucket.realBucket(), args.getKeyName());
- }
-
- metrics.incNumKeyDeletes();
- keyManager.deleteKey(args);
- AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.DELETE_KEY,
- auditMap));
- metrics.decNumKeys();
- } catch (Exception ex) {
- metrics.incNumKeyDeleteFails();
- AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.DELETE_KEY,
- auditMap, ex));
- throw ex;
- }
+ throw new UnsupportedOperationException("OzoneManager does not require " +
Review comment:
And also in similar mannner are you also planning to delete KeyManagerImpl delete method also? (I think we can completely remove that 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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r705909015
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2682,27 +2682,8 @@ public void renameKey(OmKeyArgs args, String toKeyName) throws IOException {
*/
@Override
public void deleteKey(OmKeyArgs args) throws IOException {
- Map<String, String> auditMap = args.toAuditMap();
- try {
- ResolvedBucket bucket = resolveBucketLink(args);
- args = bucket.update(args);
-
- if (isAclEnabled) {
- checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.DELETE,
- bucket.realVolume(), bucket.realBucket(), args.getKeyName());
- }
-
- metrics.incNumKeyDeletes();
- keyManager.deleteKey(args);
- AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.DELETE_KEY,
- auditMap));
- metrics.decNumKeys();
- } catch (Exception ex) {
- metrics.incNumKeyDeleteFails();
- AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.DELETE_KEY,
- auditMap, ex));
- throw ex;
- }
+ throw new UnsupportedOperationException("OzoneManager does not require " +
Review comment:
> Just a question, instead of doing this way, is there a way we can remove these methods.
I haven't looked at that much yet, because I saw that deleteKeys(), renameKeys(), setQuota() and revokeS3Secret() all use the UnsupportedOperationException approach, so I assumed that is what I was supposed to do. I just copied the code from deleteKeys()
> My question is coming from the context, as if any new write requests
> added to OzoneManagerProtocol, still we should add this
> UnsupportedOperationException?
I think so, unfortunately, until we redefine the OzoneManagerProtocol. And as @cxorm says, that is likely to be a huge patch.
> If that is too much change, we can explore that in a new Jira. Lets
> get the work going in, as I see it is causing lot of confusion to OM
> developers
I think that is the right decision. I think there are two separate problems
1. removing the old unused methods, (and fixing the tests that use them).
2. fixing the OzoneManagerProtocol
As of right now, I think they can be handled in separate PR's. And I think 1 is much higher priority, because as you've said, the code is causing so much confusion. What do you think, @bharatviswa504 ?
> And also in similar mannner are you also planning to delete
> KeyManagerImpl delete method also? (I think we can completely remove
> that method)
Yes, but I think the most efficient way to do it is to finish each class in 'org.apached.hadoop.ozone.om' one at a time.
In other words, finish removing all the methods from OzoneManager.java first. And fix all the tests that that breaks. Then all the methods from the KeyManager.java and fix the tests that those break, and so on. Does that sound ok, @bharatviswa504 ?
> I ever used to the same throw-Exception manner and I think it doesn't solve the problem fully.
So do you think it is ok to just continue on with the OM for now, and add UnsupportedOperationException's for the rest of the methods, before moving on to the other classes? Would that cause any problems @cxorm ?
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GeorgeJahad commented on pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#issuecomment-933004049
this one will be completed here:
https://github.com/apache/ozone/pull/2703
--
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@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org