YARN-11349. [Federation] Router Support DelegationToken With SQL. #5169
Conversation
This file contains 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
|
@steveloughran @ayushtkn This pr cannot be compiled by jenkins, do you know how to deal with it? I checked the code and found no problem. |
|
It is giving "java.nio.file.FileSystemException: /home/jenkins/jenkins-home/workspace/hadoop-multibranch: Read-only file system" for hadoop2 node, I don't know if it is the only node affected but I have disabled this for now: Give it a try, Need to figure out what all nodes are affected & raise an INFRA ticket. Well this happened second time, (InfrastructureINFRA-22588) New Ticket: INFRA-23953 |
|
@ayushtkn Thank you very much for your help! It seems that jenkins is running normally. I will continue to observe and report back the final running results. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Nov 29, 2022
|
|
||
| // Execute the query | ||
| long startTime = clock.getTime(); | ||
| cstmt.executeUpdate(); |
There was a problem hiding this comment.
We keep doing these blocks of codes over and over.
We could have a call with parameters for input, output, etc.
There was a problem hiding this comment.
Thank you very much for your help in reviewing the code, I will refactor this part of the code.
| @Override | ||
| protected void setTokenInfo(String tokenInfo) { | ||
| maybeInitBuilder(); | ||
| if(tokenInfo == null) { |
There was a problem hiding this comment.
BTW, how is checkstyle not catching this?
| public static void validate(RouterMasterKeyRequest request) | ||
| throws FederationStateStoreInvalidInputException { | ||
|
|
||
| // Verify the request to ensure that the request is not empty, |
...st/java/org/apache/hadoop/yarn/server/federation/store/impl/TestSQLFederationStateStore.java
Show resolved
Hide resolved
...st/java/org/apache/hadoop/yarn/server/federation/store/impl/TestSQLFederationStateStore.java
Outdated
Show resolved
Hide resolved
| public void testUpdateStoredToken() throws IOException, YarnException { | ||
| super.testUpdateStoredToken(); | ||
| FederationStateStore stateStore = getStateStore(); |
There was a problem hiding this comment.
Not much difference with FederationStateStoreBaseTest#testUpdateStoredToken
There was a problem hiding this comment.
We will use the super test directly, the super test can cover this part, I will modify the code.
| byte[] tokenInfoBytes = | ||
| RouterDelegationTokenSupport.encodeDelegationTokenInformation(tokenInfo); | ||
| long renewDate = tokenInfo.getRenewDate(); | ||
| String token = Base64.getUrlEncoder().encodeToString(tokenInfoBytes); |
There was a problem hiding this comment.
We always do this.
Why not make encodeDelegationTokenInformation() return the String encoded already?
There was a problem hiding this comment.
Thanks for your suggestion, I will modify this part of the code.
|
@goiri Thank you very much for your suggestions, I still need to spend some time (1-2 days) to improve. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| byte[] tokenInfoBytes = bos.toByteArray(); | ||
| return Base64.getUrlEncoder().encodeToString(tokenInfoBytes); | ||
| } catch (IOException ex) { | ||
| throw new RuntimeException("Failed to encode token.", ex); |
There was a problem hiding this comment.
throw UncheckedIOException; add ex.toString() to message. its pretty unlikely to happen here, but best practice
There was a problem hiding this comment.
Thank you very much for helping to review the code, I will modify the code.
| if (!shouldIgnoreException(e)) { | ||
| LOG.error("Error in storing RMDelegationToken with sequence number: {}.", | ||
| identifier.getSequenceNumber()); | ||
| ExitUtil.terminate(1, e); |
There was a problem hiding this comment.
this really what should be done?
There was a problem hiding this comment.
Thank you for your question. In YARN Federation, for Client, Router plays a role similar to RM. Our code here refers to the code of RM. From my personal point of view, I think it should still maintain the same processing logic as RM, which will be better.
RMDelegationTokenSecretManager#storeNewToken
protected void storeNewToken(RMDelegationTokenIdentifier identifier,
long renewDate) {
try {
LOG.info("storing RMDelegation token with sequence number: "
+ identifier.getSequenceNumber());
rm.getRMContext().getStateStore().storeRMDelegationToken(identifier,
renewDate);
} catch (Exception e) {
if (!shouldIgnoreException(e)) {
LOG.error("Error in storing RMDelegationToken with sequence number: "
+ identifier.getSequenceNumber());
ExitUtil.terminate(1, e);
}
}
}
|
💔 -1 overall
This message was automatically generated. |
|
@goiri Can you help to review this PR again? Thank you very much! |
goiri
reviewed
Dec 6, 2022
| * @param masterKey masterKey. | ||
| * @return DelegationKey. | ||
| */ | ||
| private DelegationKey convertMasterKeyToDelegationKey(RouterMasterKey masterKey) { |
There was a problem hiding this comment.
Thank you very much for helping to review the code! I will fix it.
| * RowCount Handler. | ||
| * Used to parse out the rowCount information of the output parameter. | ||
| */ | ||
| public class RowCountHandler implements ResultSetHandler<Integer> { |
There was a problem hiding this comment.
This class is SQL specific right? we should have it in the name or in a sql package.
There was a problem hiding this comment.
I will create sql folder and move related classes to sql folder.
| @@ -558,38 +557,37 @@ public void testDeleteReservationHomeSubClusterAbnormalSituation() throws Except | |||
| () -> stateStore.deleteReservationHomeSubCluster(delRequest)); | |||
| } | |||
|
|
|||
| @Test(expected = NotImplementedException.class) | |||
| @Test | |||
| public void testStoreNewMasterKey() throws Exception { | |||
There was a problem hiding this comment.
I think by default this gets executed right? It already has the Test annotation on the parent.
We can get rid of this.
(Check in the next run that this test actually runs.)
There was a problem hiding this comment.
I agree with you, this part of the code can be removed, and the super class can already cover this part by running Junit Test.
goiri
approved these changes
Dec 7, 2022
| try { | ||
| FederationQueryRunner runner = new FederationQueryRunner(); | ||
| FederationSQLOutParameter<String> tokenIdentOUT = | ||
| new FederationSQLOutParameter<>("tokenIdent_OUT", java.sql.Types.VARCHAR, String.class); |
There was a problem hiding this comment.
Thank you very much for helping to review the code, I will fix it.
| */ | ||
| public void close(Statement stmt) throws SQLException { | ||
| if (stmt != null) { | ||
| stmt.close(); |
| @Override | ||
| public RouterMasterKey handle(Object... params) throws SQLException { | ||
| RouterMasterKey routerMasterKey = Records.newRecord(RouterMasterKey.class); | ||
| for (int i = 0; i < params.length; i++) { |
There was a problem hiding this comment.
Thanks for your suggestion, I will modify the code.
| @Override | ||
| public Integer handle(Object... params) throws SQLException { | ||
| Integer result = 0; | ||
| for (int i = 0; i < params.length; i++) { |
| Integer result = 0; | ||
| for (int i = 0; i < params.length; i++) { | ||
| if (params[i] != null) { | ||
| if (params[i] instanceof FederationSQLOutParameter) { |
There was a problem hiding this comment.
I think instanceof already checks for null, otherwise you can do an &&
|
💔 -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. |
|
💔 -1 overall
This message was automatically generated. |
|
@goiri Thank you very much for helping to review the code! I still need 1-2 days to improve this pr. |
…p file after encountering an IOException. (apache#5145)
…server reads without using the ObserverReadProxyProvider. (apache#5142)
…ache#5197) Contributed by Oleksandr Shevchenko
…own nodes should be 0 now expected: <1> but was: <0> (apache#5190) Reviewed-by: Peter Szucs Signed-off-by: Chris Nauroth <cnauroth@apache.org>
Reviewed-by: Chris Nauroth <cnauroth@apache.org> Reviewed-by: slfan1989 <55643692+slfan1989@users.noreply.github.com> Signed-off-by: Tao Li <tomscut@apache.org>
This is a followup to the original HADOOP-18546 patch; cherry-picks of that should include this or follow up with it. Removes risk of race conditions in assertions of ITestReadBufferManager on the state of the in-progress and completed queues by removing assertions brittle to race conditions in scheduling/network IO * Waits for all the executor pool shutdown to complete before making any assertions * Assertions that there are no in progress reads MUST be cut as there may be some and they won't be cancelled. * Assertions that the completed list is without buffers of a closed stream are brittle because if there was an in progress stream which completed after stream.close() then it will end up in the list. Contributed by Steve Loughran
…ges (apache#4537) The static boolean PlatformName.IBM_JAVA now identifies Java 11+ IBM Semeru runtimes as IBM JVM releases. Contributed by Jack Buggins.
…esystem. (apache#5206). Contributed by Beibei Zhao. Signed-off-by: Chris Nauroth <cnauroth@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
… org.apache.hadoop.mapreduce.v2.app.webapp (apache#5209) Contributed by: Ashutosh Gupta
…ts. (apache#5110) The start and end of the range is set in a new audit param "rg", e.g "?rg=100-200" Contributed by Ankit Saurabh
(cherry picked from commit df4812d)
… references (apache#5144) This has triggered an OOM in a process which was churning through s3a fs instances; the increased memory footprint of IOStatistics amplified what must have been a long-standing issue with FS instances being created and not closed() * Makes sure instrumentation is closed when the FS is closed. * Uses a weak reference from metrics to instrumentation, so even if the FS wasn't closed (see HADOOP-18478), this back reference would not cause the S3AInstrumentation reference to be retained. * If S3AFileSystem is configured to log at TRACE it will log the calling stack of initialize(), so help identify where the instance is being created. This should help track down the cause of instance leakage. Contributed by Steve Loughran.
…e DEBUG logs less noisy (apache#5223) Contributed by: Mehakmeet Singh
…pache#5221) The kerberos RPC does not declare any restriction on characters used in kerberos names, though implementations MAY be more restrictive. If the kerberos controller supports use non-conventional principal names *and the kerberos admin chooses to use them* this can confuse some of the parsing. The obvious solution is for the enterprise admins to "not do that" as a lot of things break, bits of hadoop included. Harden the hadoop code slightly so at least we fail more gracefully, so people can then get in touch with their sysadmin and tell them to stop it.
Addresses CVE-2021-37533, which *only* relates to FTP. Applications not using the ftp:// filesystem, which, as anyone who has used it will know is very minimal and so rarely used, is not a critical part of the project. Furthermore, the FTP-related issue is at worst information leakage if someone connects to a malicious server. This is a due diligence PR rather than an emergency fix. Contributed by Steve Loughran
Followup patch to HADOOP-18456 as part of HADOOP-18521,
ABFS ReadBufferManager buffer sharing across concurrent HTTP requests
Add probes of readahead fix aid in checking safety of
hadoop ABFS client across different releases.
* ReadBufferManager constructor logs the fact it is safe at TRACE
* AbfsInputStream declares it is fixed in toString()
by including fs.azure.capability.readahead.safe" in the
result.
The ABFS FileSystem hasPathCapability("fs.azure.capability.readahead.safe")
probe returns true to indicate the client's readahead manager has been fixed
to be safe when prefetching.
All Hadoop releases for which probe this returns false
and for which the probe "fs.capability.etags.available"
returns true at risk of returning invalid data when reading
ADLS Gen2/Azure storage data.
Contributed by Steve Loughran.
Signed-off-by: Tao Li <tomscut@apache.org>
…dy shutting down (apache#5160) Signed-off-by: Erik Krogen <xkrogen@apache.org>
…rs in cases of infrequent logging (apache#5215) Signed-off-by: Erik Krogen <xkrogen@apache.org> Co-authored-by: Chengbing Liu <liuchengbing@qiyi.com>
Fixes a javadoc error which came with HADOOP-18577. ABFS: Add probes of readahead fix (apache#5205) Part of the HADOOP-18521 ABFS readahead fix; MUST be included. Contributed by Steve Loughran
Due diligence followup to HADOOP-18469. Add secure XML parser factories to XMLUtils (apache#4940) Contributed by P J Fanning
slfan1989
force-pushed
the
YARN-11349-V2
branch
from
69db1eb
to
8730458
Compare
|
@slfan1989 please cancel this PR and remove the link from the JIRA. |
Thank you very much! I will close this pr and remove the link from the JIRA. |
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.
JIRA: YARN-11349. [Federation] Router Support DelegationToken With SQL.
We will create a sequenceTable table, this table will store the values of sequenceNum and currentKeyId.