You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/08/10 17:46:52 UTC
[GitHub] [incubator-gobblin] sv2000 opened a new pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
sv2000 opened a new pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [x] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1228
### Description
- [x] Here are some details about my PR, including screenshots (if applicable):
Currently, the token file is localized on each new container launch. The localization should happen only for AM container. Further, this task also provides a fix for NPE thrown in case the token file is not available locally.
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Added unit tests.
### Commits
- [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] asfgit closed pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] ZihanLi58 removed a comment on pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
Posted by GitBox <gi...@apache.org>.
ZihanLi58 removed a comment on pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074#issuecomment-671518204
+1, LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074#discussion_r468084022
##########
File path: gobblin-yarn/src/test/java/org/apache/gobblin/yarn/GobblinYarnTestUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.yarn;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FileSystemTestHelper;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.io.Text;
+import org.apache.hadoop.security.Credentials;
+import org.apache.hadoop.security.token.Token;
+import org.apache.hadoop.security.token.TokenIdentifier;
+import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+/**
+ * Utility class
+ */
+public class GobblinYarnTestUtils {
+ /**
+ * A utility method for generating a {@link org.apache.hadoop.fs.FileSystemTestHelper.MockFileSystem} instance
+ * that can return a delegation token on {@link org.apache.hadoop.fs.FileSystem#getDelegationToken(String)}.
+ *
+ * @param service
+ * @return
+ * @throws IOException
+ */
+ public static FileSystemTestHelper.MockFileSystem createFileSystemForServiceName(final String service)
+ throws IOException {
+ FileSystemTestHelper.MockFileSystem mockFs = new FileSystemTestHelper.MockFileSystem();
+ Mockito.when(mockFs.getCanonicalServiceName()).thenReturn(service);
+ Mockito.when(mockFs.getDelegationToken(Mockito.any(String.class))).thenAnswer(new Answer<Token<?>>() {
+ int unique = 0;
+ @Override
+ public Token<?> answer(InvocationOnMock invocation) throws Throwable {
+ Token<?> token = new Token<TokenIdentifier>();
+ token.setService(new Text(service));
+ // use unique value so when we restore from token storage, we can
+ // tell if it's really the same token
+ token.setKind(new Text("token" + unique++));
+ return token;
+ }
+ });
+ return mockFs;
+ }
+
+ /**
+ * Writes a token file to a given path.
+ * @param path
+ * @param serviceName
+ * @throws IOException
+ */
+ public static void createTokenFileForService(Path path, String serviceName)
Review comment:
This is a utility method I used for creating the .token file added to the test resources folder. It can be potentially leveraged in the future.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
Posted by GitBox <gi...@apache.org>.
sv2000 commented on pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074#issuecomment-671496187
@ZihanLi58 @autumnust please review.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] ZihanLi58 commented on a change in pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on a change in pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074#discussion_r468076303
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java
##########
@@ -72,19 +73,23 @@ public static void writeTokenToFile(Token<? extends TokenIdentifier> token, Path
}
/**
- * Update {@link Token} with token file in resources.
+ * Update {@link Token} with token file localized by NM.
*
- * @param
+ * @param tokenFileName name of the token file
* @throws IOException
*/
- public static void updateToken() throws IOException{
- File tokenFile = new File(YarnHelixUtils.class.getClassLoader().getResource(GobblinYarnConfigurationKeys.TOKEN_FILE_NAME).getFile());
- if(tokenFile.exists()) {
- Credentials credentials = Credentials.readTokenStorageFile(tokenFile, new Configuration());
- for (Token<? extends TokenIdentifier> token : credentials.getAllTokens()) {
- LOGGER.info("updating " + token.getKind() + " " + token.getService());
+ public static void updateToken(String tokenFileName) throws IOException{
+ URL tokenFileUrl = YarnHelixUtils.class.getClassLoader().getResource(tokenFileName);
+ if (tokenFileUrl != null) {
+ File tokenFile = new File(
+ YarnHelixUtils.class.getClassLoader().getResource(GobblinYarnConfigurationKeys.TOKEN_FILE_NAME).getFile());
Review comment:
Can we reuse the tokenFileUrl to initialize the tokenFile?
##########
File path: gobblin-yarn/src/test/java/org/apache/gobblin/yarn/GobblinYarnTestUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.yarn;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FileSystemTestHelper;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.io.Text;
+import org.apache.hadoop.security.Credentials;
+import org.apache.hadoop.security.token.Token;
+import org.apache.hadoop.security.token.TokenIdentifier;
+import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+/**
+ * Utility class
+ */
+public class GobblinYarnTestUtils {
+ /**
+ * A utility method for generating a {@link org.apache.hadoop.fs.FileSystemTestHelper.MockFileSystem} instance
+ * that can return a delegation token on {@link org.apache.hadoop.fs.FileSystem#getDelegationToken(String)}.
+ *
+ * @param service
+ * @return
+ * @throws IOException
+ */
+ public static FileSystemTestHelper.MockFileSystem createFileSystemForServiceName(final String service)
+ throws IOException {
+ FileSystemTestHelper.MockFileSystem mockFs = new FileSystemTestHelper.MockFileSystem();
+ Mockito.when(mockFs.getCanonicalServiceName()).thenReturn(service);
+ Mockito.when(mockFs.getDelegationToken(Mockito.any(String.class))).thenAnswer(new Answer<Token<?>>() {
+ int unique = 0;
+ @Override
+ public Token<?> answer(InvocationOnMock invocation) throws Throwable {
+ Token<?> token = new Token<TokenIdentifier>();
+ token.setService(new Text(service));
+ // use unique value so when we restore from token storage, we can
+ // tell if it's really the same token
+ token.setKind(new Text("token" + unique++));
+ return token;
+ }
+ });
+ return mockFs;
+ }
+
+ /**
+ * Writes a token file to a given path.
+ * @param path
+ * @param serviceName
+ * @throws IOException
+ */
+ public static void createTokenFileForService(Path path, String serviceName)
Review comment:
Where does this method been called?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] jhsenjaliya commented on a change in pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
Posted by GitBox <gi...@apache.org>.
jhsenjaliya commented on a change in pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074#discussion_r468106247
##########
File path: gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java
##########
@@ -72,19 +73,22 @@ public static void writeTokenToFile(Token<? extends TokenIdentifier> token, Path
}
/**
- * Update {@link Token} with token file in resources.
+ * Update {@link Token} with token file localized by NM.
*
- * @param
+ * @param tokenFileName name of the token file
* @throws IOException
*/
- public static void updateToken() throws IOException{
- File tokenFile = new File(YarnHelixUtils.class.getClassLoader().getResource(GobblinYarnConfigurationKeys.TOKEN_FILE_NAME).getFile());
- if(tokenFile.exists()) {
- Credentials credentials = Credentials.readTokenStorageFile(tokenFile, new Configuration());
- for (Token<? extends TokenIdentifier> token : credentials.getAllTokens()) {
- LOGGER.info("updating " + token.getKind() + " " + token.getService());
+ public static void updateToken(String tokenFileName) throws IOException{
+ URL tokenFileUrl = YarnHelixUtils.class.getClassLoader().getResource(tokenFileName);
+ if (tokenFileUrl != null) {
Review comment:
+1 on NPE check, this was an issue on previous code. Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] ZihanLi58 commented on pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074#issuecomment-671518204
+1, LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074#discussion_r468169733
##########
File path: gobblin-yarn/src/test/java/org/apache/gobblin/yarn/GobblinYarnTestUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.yarn;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FileSystemTestHelper;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.io.Text;
+import org.apache.hadoop.security.Credentials;
+import org.apache.hadoop.security.token.Token;
+import org.apache.hadoop.security.token.TokenIdentifier;
+import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+/**
+ * Utility class
+ */
+public class GobblinYarnTestUtils {
+ /**
+ * A utility method for generating a {@link org.apache.hadoop.fs.FileSystemTestHelper.MockFileSystem} instance
+ * that can return a delegation token on {@link org.apache.hadoop.fs.FileSystem#getDelegationToken(String)}.
+ *
+ * @param service
+ * @return
+ * @throws IOException
+ */
+ public static FileSystemTestHelper.MockFileSystem createFileSystemForServiceName(final String service)
+ throws IOException {
+ FileSystemTestHelper.MockFileSystem mockFs = new FileSystemTestHelper.MockFileSystem();
+ Mockito.when(mockFs.getCanonicalServiceName()).thenReturn(service);
+ Mockito.when(mockFs.getDelegationToken(Mockito.any(String.class))).thenAnswer(new Answer<Token<?>>() {
+ int unique = 0;
+ @Override
+ public Token<?> answer(InvocationOnMock invocation) throws Throwable {
+ Token<?> token = new Token<TokenIdentifier>();
+ token.setService(new Text(service));
+ // use unique value so when we restore from token storage, we can
+ // tell if it's really the same token
+ token.setKind(new Text("token" + unique++));
+ return token;
+ }
+ });
+ return mockFs;
+ }
+
+ /**
+ * Writes a token file to a given path.
+ * @param path
+ * @param serviceName
+ * @throws IOException
+ */
+ public static void createTokenFileForService(Path path, String serviceName)
Review comment:
Should we considering calling this method as part of test to avoid binary file (which brought in .crc as well) as part of PR ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3074: GOBBLIN-1228: Do not localize token file on new TaskRunner launch
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3074:
URL: https://github.com/apache/incubator-gobblin/pull/3074#discussion_r468173008
##########
File path: gobblin-yarn/src/test/java/org/apache/gobblin/yarn/GobblinYarnTestUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.yarn;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FileSystemTestHelper;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.io.Text;
+import org.apache.hadoop.security.Credentials;
+import org.apache.hadoop.security.token.Token;
+import org.apache.hadoop.security.token.TokenIdentifier;
+import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+/**
+ * Utility class
+ */
+public class GobblinYarnTestUtils {
+ /**
+ * A utility method for generating a {@link org.apache.hadoop.fs.FileSystemTestHelper.MockFileSystem} instance
+ * that can return a delegation token on {@link org.apache.hadoop.fs.FileSystem#getDelegationToken(String)}.
+ *
+ * @param service
+ * @return
+ * @throws IOException
+ */
+ public static FileSystemTestHelper.MockFileSystem createFileSystemForServiceName(final String service)
+ throws IOException {
+ FileSystemTestHelper.MockFileSystem mockFs = new FileSystemTestHelper.MockFileSystem();
+ Mockito.when(mockFs.getCanonicalServiceName()).thenReturn(service);
+ Mockito.when(mockFs.getDelegationToken(Mockito.any(String.class))).thenAnswer(new Answer<Token<?>>() {
+ int unique = 0;
+ @Override
+ public Token<?> answer(InvocationOnMock invocation) throws Throwable {
+ Token<?> token = new Token<TokenIdentifier>();
+ token.setService(new Text(service));
+ // use unique value so when we restore from token storage, we can
+ // tell if it's really the same token
+ token.setKind(new Text("token" + unique++));
+ return token;
+ }
+ });
+ return mockFs;
+ }
+
+ /**
+ * Writes a token file to a given path.
+ * @param path
+ * @param serviceName
+ * @throws IOException
+ */
+ public static void createTokenFileForService(Path path, String serviceName)
Review comment:
Currently, the method uses ClassLoader#getResource to locate the file. So the test will have to add the resource dynamically into classpath, which involves hacky workarounds.
https://stackoverflow.com/questions/1010919/adding-files-to-java-classpath-at-runtime
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org