Skip to content

Conversation

@amaliujia
Copy link
Contributor

@amaliujia amaliujia commented Sep 1, 2020

What changes were proposed in this pull request?

Change ozone admin om getserviceroles to ozone admin om roles

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4189

How was this patch tested?

Unit Test

@amaliujia
Copy link
Contributor Author

R: @adoroszlai @timmylicheng

@amaliujia
Copy link
Contributor Author

Context: #1346 (comment)

Copy link
Member

@cxorm cxorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amaliujia for the work.

This modification looks good to me.

@amaliujia
Copy link
Contributor Author

Thank you for your review @cxorm!

[ERROR] Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 4.417 s <<< FAILURE! - in org.apache.hadoop.ozone.om.request.key.TestOMAllocateBlockRequest
[ERROR] testValidateAndUpdateCache(org.apache.hadoop.ozone.om.request.key.TestOMAllocateBlockRequest)  Time elapsed: 0.114 s  <<< FAILURE!
java.lang.AssertionError: Values should be different. Actual: 1599003025036
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failEquals(Assert.java:185)
	at org.junit.Assert.assertNotEquals(Assert.java:161)
	at org.junit.Assert.assertNotEquals(Assert.java:198)
	at org.junit.Assert.assertNotEquals(Assert.java:209)
	at org.apache.hadoop.ozone.om.request.key.TestOMAllocateBlockRequest.testValidateAndUpdateCache(TestOMAllocateBlockRequest.java:100)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)

The failed UT seems not related to this change.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amaliujia for working on this. Can you please add an alias to avoid breaking any existing scripts that might use this command?

Co-authored-by: Doroszlai, Attila <[email protected]>
@amaliujia
Copy link
Contributor Author

It seemed that there is another opinion about what command name should be. So I will convert this PR to a draft now to wait for a consensus.

Thanks for all reviews so far and sorry for the confusion.

@amaliujia amaliujia marked this pull request as draft September 2, 2020 04:01
@amaliujia amaliujia changed the title HDDS-4189. Change ozone admin om getserviceroles to ozone admin om status HDDS-4189. Change ozone admin om getserviceroles to ozone admin om roles Sep 2, 2020
@amaliujia amaliujia marked this pull request as ready for review September 2, 2020 23:57
@amaliujia
Copy link
Contributor Author

@adoroszlai

Ok finally I think we can use roles based on #1346 (comment).

I made necessary change in this PR. Can you take another look?

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amaliujia for this improvement and @cxorm for the review.

@adoroszlai adoroszlai merged commit b2fca43 into apache:master Sep 3, 2020
@amaliujia amaliujia deleted the HDDS-4189 branch September 3, 2020 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants