Georges suggestions #8
Conversation
| @@ -84,7 +86,7 @@ public OmTestManagers(OzoneConfiguration conf, | |||
| blockClient = | |||
| new ScmBlockLocationTestingClient(null, null, 0); | |||
| } | |||
|
|
|||
| @@ -116,7 +118,15 @@ public OmTestManagers(OzoneConfiguration conf, | |||
| .getInternalState(om, "bucketManager"); | |||
| prefixManager = (PrefixManagerImpl)HddsWhiteboxTestUtils | |||
| .getInternalState(om, "prefixManager"); | |||
| } | |||
|
|
|||
| public KeyProviderCryptoExtension kmsProviderInit() { | |||
There was a problem hiding this comment.
add a comment something like:
// returns a mock kmsProvider
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
| import org.junit.Rule; | ||
|
|
There was a problem hiding this comment.
why did you add these here and remove them below?
There was a problem hiding this comment.
Changed that as well to minimize the diffs
|
|
||
| import org.apache.hadoop.crypto.key.KeyProvider; | ||
| import org.apache.hadoop.crypto.key.KeyProviderCryptoExtension; | ||
| import org.apache.hadoop.hdds.client.StandaloneReplicationConfig; | ||
| import org.apache.hadoop.hdds.conf.OzoneConfiguration; | ||
| import org.apache.hadoop.hdds.protocol.StorageType; | ||
| import org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor; | ||
| import org.apache.hadoop.hdds.protocol.proto.HddsProtos; |
There was a problem hiding this comment.
It seems that the IDE altered it because I was making a static reference to ReplicationFactor like HddsProtos.ReplicationFactor, but I changed it to minimize the changes.
| @@ -67,51 +85,51 @@ private OzoneConfiguration createNewTestPath() throws IOException { | |||
| return conf; | |||
| } | |||
|
|
|||
| private OmMetadataManagerImpl createSampleVol() throws IOException { | |||
| private void createSampleVol() throws IOException, AuthenticationException { | |||
There was a problem hiding this comment.
Does this really need the AuthenticationException?
There was a problem hiding this comment.
We need that due to initialization of OmTestManagers. If I remove that there is an error from the IDE.
| try { | ||
| BucketManager bucketManager = new BucketManagerImpl(metaMgr); | ||
| OzoneManagerProtocol writeCl = omTestManagers.getWriteClient(); |
There was a problem hiding this comment.
we've used writeClient everywhere else. I would use that instead of writeCl here.
It's good to be consistent unless there is a good reason otherwise.
There was a problem hiding this comment.
I declared OzoneManagerProtocol as well based on your PR and then I was getting an error from checkstyle about hiding the class field with the same name. But now I realize there is a so much better way and simpler way because the class field has not been initialized. I will fix that.
| .setBucketEncryptionKey(new | ||
| BucketEncryptionKeyInfo.Builder().setKeyName("key1").build()) | ||
| BucketEncryptionKeyInfo.Builder().setKeyName("key1").build()) |
|
|
||
| OmBucketInfo bucketInfoRead = | ||
| bucketManager.getBucketInfo("sampleVol", "bucketOne"); | ||
| bucketManager.getBucketInfo("sample-vol", "bucket-one"); |
|
|
||
| Assert.assertTrue(bucketInfoRead.getEncryptionKeyInfo().getKeyName() | ||
| .equals(bucketInfo.getEncryptionKeyInfo().getKeyName())); | ||
| metaMgr.getStore().close(); | ||
| .equals(bucketInfo.getEncryptionKeyInfo().getKeyName())); |
| } | ||
|
|
||
| private OzoneConfiguration createNewTestPath() throws IOException, | ||
| AuthenticationException { |
There was a problem hiding this comment.
Does this really need the AuthenticationException?
There was a problem hiding this comment.
That's actually not needed, I will remove it. I declared inside the method omTestManagers but when I removed the field, I forgot about it.
No description provided.