HDDS-1119. DN get OM certificate from SCM CA for block token validat… #601
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
xiaoyuyao
reviewed
Mar 13, 2019
| /** | ||
| * Returns serial id of SCM issued certificate. | ||
| * | ||
| * @return IP address |
There was a problem hiding this comment.
NIT: @return should be updated.
xiaoyuyao
reviewed
Mar 13, 2019
| /** | ||
| * Returns serial id of SCM issued certificate. | ||
| * | ||
| * @return IP address |
There was a problem hiding this comment.
Javadoc copy paste error like above, this one set and does not return.
xiaoyuyao
reviewed
Mar 13, 2019
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/token/BlockTokenVerifier.java
Outdated
Show resolved
Hide resolved
02166c1 to
2b742fd
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
a015644 to
0b57d76
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
e44820d to
02392da
Compare<
8000
/span>
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
xiaoyuyao
reviewed
Mar 15, 2019
| return null; | ||
| } | ||
| // Refresh the cache from file system. | ||
| loadAllCertificates(); |
There was a problem hiding this comment.
Should we loadAllCertificates() again only if the map does not contain it?
Also, in the contsructor when we call loadAllCertificates(), should we asset the passing in certSerialId is loaded into the map from file system?
There was a problem hiding this comment.
initialized local certificate during initial call to loadAllCertificates. Now if it is null at L202 than it is also not present in map, so loading it again from filesystem and checking it again is the only option.
There was a problem hiding this comment.
We can assert in constructor if certSerialId is not null than its corresponding certificate should not be null as well after loading from memory but if we fail than it will become catch 22 for some recovery scenarios. (Ex we can't even create instance of CertificateClient to call init which may handle some of the automatic recovery in future)
There was a problem hiding this comment.
Let's discuss and file follow up JIRAs. I'm OK with the current approach as-is.
xiaoyuyao
reviewed
Mar 15, 2019
...n/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java
Outdated
Show resolved
Hide resolved
xiaoyuyao
reviewed
Mar 15, 2019
hadoop-hdds/common/src/main/java/org/apache/hadoop/utils/db/LongCodec.java
Outdated
Show resolved
Hide resolved
xiaoyuyao
reviewed
Mar 15, 2019
| @@ -90,8 +90,9 @@ public OzoneDelegationTokenSecretManager(OzoneConfiguration conf, | |||
| service, LOG); | |||
| currentTokens = new ConcurrentHashMap(); | |||
| this.tokenRemoverScanInterval = dtRemoverScanInterval; | |||
| this.store = new OzoneSecretStore(conf); | |||
| this.s3SecretManager = s3SecretManager; | |||
| this.s3SecretManager = (S3SecretManagerImpl) s3SecretManager; | |||
There was a problem hiding this comment.
why do we need to change s3secret manager here from interface to impl? seems not relate to this ticket.
There was a problem hiding this comment.
S3SecretManagerImpl has helper function to return OMMetadataManager required in secret store. Its interface doesn't. Since it is returning a dependency i think its better to not push it to interface itself.
|
Thanks @ajayydv for the update. We are almost there. Just few last issues as commented inline. |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks @ajayydv for the update. +1 the latest push, pending fix of the related findbugs issue and unit test results. |
|
💔 -1 overall
This message was automatically generated. |
bb81529 to
9f59f08
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
…ion. Contributed by Ajay Kumar.
|
💔 -1 overall
This message was automatically generated. |
|
@xiaoyuyao thanks for continuous reviews of this long patch. UT failures looks unrelated. |
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
- Fix getSystemStreamMetadata in InMemoryManager to filter based on the system name on top of stream names Author: Bharath Kumarasubramanian <bkumaras@linkedin.com> Reviewers: Xinyu Liu <xinyuiscool@github.com> Closes apache#601 from bharathkk/in-memory-fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ion. Contributed by Ajay Kumar.