Skip to content

Conversation

@sguggilam
Copy link
Contributor

@sguggilam sguggilam commented Aug 6, 2020

The commit was reverted and a new PR for this JIRA is posted as #2249

@sguggilam
Copy link
Contributor Author

@liuml07 @steveloughran Please review

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 33s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 💚 mvninstall 29m 18s trunk passed
+1 💚 compile 21m 53s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 compile 18m 35s trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚 checkstyle 0m 51s trunk passed
+1 💚 mvnsite 1m 25s trunk passed
+1 💚 shadedclient 16m 19s branch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 35s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 1m 31s trunk passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+0 🆗 spotbugs 2m 15s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 2m 13s trunk passed
_ Patch Compile Tests _
+1 💚 mvninstall 0m 54s the patch passed
+1 💚 compile 21m 1s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javac 21m 1s the patch passed
+1 💚 compile 18m 24s the patch passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚 javac 18m 24s the patch passed
-0 ⚠️ checkstyle 0m 47s hadoop-common-project/hadoop-common: The patch generated 1 new + 85 unchanged - 0 fixed = 86 total (was 85)
+1 💚 mvnsite 1m 25s the patch passed
-1 ❌ whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
-1 ❌ whitespace 0m 0s The patch 1 line(s) with tabs.
+1 💚 shadedclient 14m 11s patch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 32s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 1m 35s the patch passed with JDK Private Build-1.8.0_252-8u252-b09-1~18.04-b09
+1 💚 findbugs 2m 26s the patch passed
_ Other Tests _
+1 💚 unit 9m 16s hadoop-common in the patch passed.
+1 💚 asflicense 0m 50s The patch does not generate ASF License warnings.
167m 10s
Subsystem Report/Notes
Docker ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/1/artifact/out/Dockerfile
GITHUB PR #2197
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux ab89126be726 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 1d5ccc7
Default Java Private Build-1.8.0_252-8u252-b09-1~18.04-b09
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_252-8u252-b09-1~18.04-b09
checkstyle https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/1/artifact/out/diff-checkstyle-hadoop-common-project_hadoop-common.txt
whitespace https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/1/artifact/out/whitespace-eol.txt
whitespace https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/1/artifact/out/whitespace-tabs.txt
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/1/testReport/
Max. process+thread count 2475 (vs. ulimit of 5500)
modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/1/console
versions git=2.17.1 maven=3.6.0 findbugs=4.0.6
Powered by Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@steveloughran
Copy link
Contributor

LGTM, though I'll leave to someone who understands UGI to give the final vote

@liuml07 liuml07 requested review from arp7 and kihwal August 7, 2020 18:02
@liuml07
Copy link
Member

liuml07 commented Aug 7, 2020

Thanks @steveloughran I added Arpit and kihwal as reviewers.

@arp7 arp7 requested a review from xiaoyuyao August 7, 2020 18:17
@sguggilam
Copy link
Contributor Author

@arp7 @kihwal @xiaoyuyao Can you please review and provide your feedback ?

@liuml07
Copy link
Member

liuml07 commented Aug 16, 2020

The patch looks good to me overall.

A question is: should we rename the word "force" with "ignoreTimeElapsed" to make it clear. If agreed on it includes updating the parameters name and Javadoc.

Thoughts? @sguggilam

I will give a final review next week.

@sguggilam
Copy link
Contributor Author

Yes @liuml07 that makes sense, let me update the PR

Copy link
Member

@liuml07 liuml07 left a comment

Choose a reason for hiding this comment

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

+1

I will commit by the end of this week if no more comments.

Copy link
Member

Choose a reason for hiding this comment

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

nit: Fore you mean Force?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes my bad, it's a typo, fixed it

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 26m 11s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 💚 mvninstall 30m 11s trunk passed
+1 💚 compile 20m 37s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 compile 18m 24s trunk passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 checkstyle 0m 52s trunk passed
+1 💚 mvnsite 1m 34s trunk passed
+1 💚 shadedclient 17m 4s branch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 36s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 1m 32s trunk passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+0 🆗 spotbugs 2m 13s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 2m 11s trunk passed
_ Patch Compile Tests _
+1 💚 mvninstall 0m 53s the patch passed
+1 💚 compile 20m 30s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javac 20m 30s the patch passed
+1 💚 compile 18m 24s the patch passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 javac 18m 24s the patch passed
-0 ⚠️ checkstyle 0m 47s hadoop-common-project/hadoop-common: The patch generated 1 new + 85 unchanged - 0 fixed = 86 total (was 85)
+1 💚 mvnsite 1m 22s the patch passed
-1 ❌ whitespace 0m 0s The patch 1 line(s) with tabs.
+1 💚 shadedclient 13m 59s patch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 30s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 1m 29s the patch passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 findbugs 2m 19s the patch passed
_ Other Tests _
+1 💚 unit 9m 52s hadoop-common in the patch passed.
+1 💚 asflicense 0m 55s The patch does not generate ASF License warnings.
191m 58s
Subsystem Report/Notes
Docker ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/2/artifact/out/Dockerfile
GITHUB PR #2197
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux d06ad23d11e8 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / b367942
Default Java Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
checkstyle https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/2/artifact/out/diff-checkstyle-hadoop-common-project_hadoop-common.txt
whitespace https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/2/artifact/out/whitespace-tabs.txt
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/2/testReport/
Max. process+thread count 1378 (vs. ulimit of 5500)
modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/2/console
versions git=2.17.1 maven=3.6.0 findbugs=4.0.6
Powered by Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 18m 39s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 💚 mvninstall 29m 57s trunk passed
+1 💚 compile 21m 37s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 compile 17m 50s trunk passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 checkstyle 0m 51s trunk passed
+1 💚 mvnsite 1m 26s trunk passed
+1 💚 shadedclient 17m 8s branch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 32s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 1m 28s trunk passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+0 🆗 spotbugs 2m 21s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 2m 19s trunk passed
_ Patch Compile Tests _
+1 💚 mvninstall 0m 53s the patch passed
+1 💚 compile 20m 58s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javac 20m 58s the patch passed
+1 💚 compile 17m 44s the patch passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 javac 17m 44s the patch passed
-0 ⚠️ checkstyle 0m 50s hadoop-common-project/hadoop-common: The patch generated 1 new + 85 unchanged - 0 fixed = 86 total (was 85)
+1 💚 mvnsite 1m 23s the patch passed
-1 ❌ whitespace 0m 0s The patch 1 line(s) with tabs.
+1 💚 shadedclient 15m 30s patch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 31s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 1m 27s the patch passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 findbugs 2m 23s the patch passed
_ Other Tests _
+1 💚 unit 9m 43s hadoop-common in the patch passed.
+1 💚 asflicense 0m 55s The patch does not generate ASF License warnings.
185m 57s
Subsystem Report/Notes
Docker ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/3/artifact/out/Dockerfile
GITHUB PR #2197
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux cf74f1a7feac 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / b367942
Default Java Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
checkstyle https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/3/artifact/out/diff-checkstyle-hadoop-common-project_hadoop-common.txt
whitespace https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/3/artifact/out/whitespace-tabs.txt
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/3/testReport/
Max. process+thread count 2881 (vs. ulimit of 5500)
modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/3/console
versions git=2.17.1 maven=3.6.0 findbugs=4.0.6
Powered by Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@hadoop-yetus
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 26m 6s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 💚 mvninstall 28m 47s trunk passed
+1 💚 compile 19m 29s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 compile 16m 54s trunk passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 checkstyle 0m 57s trunk passed
+1 💚 mvnsite 1m 30s trunk passed
+1 💚 shadedclient 16m 51s branch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 40s trunk passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 1m 36s trunk passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+0 🆗 spotbugs 2m 17s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 2m 15s trunk passed
_ Patch Compile Tests _
+1 💚 mvninstall 0m 51s the patch passed
+1 💚 compile 18m 47s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javac 18m 47s the patch passed
+1 💚 compile 16m 47s the patch passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 javac 16m 47s the patch passed
+1 💚 checkstyle 0m 53s the patch passed
+1 💚 mvnsite 1m 26s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 shadedclient 14m 7s patch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 38s the patch passed with JDK Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1
+1 💚 javadoc 1m 36s the patch passed with JDK Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
+1 💚 findbugs 2m 18s the patch passed
_ Other Tests _
+1 💚 unit 9m 19s hadoop-common in the patch passed.
+1 💚 asflicense 0m 55s The patch does not generate ASF License warnings.
185m 11s
Subsystem Report/Notes
Docker ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/4/artifact/out/Dockerfile
GITHUB PR #2197
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 09b41347c9b5 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/bin/hadoop.sh
git revision trunk / 64f36b9
Default Java Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.8+10-post-Ubuntu-0ubuntu118.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_265-8u265-b01-0ubuntu2~18.04-b01
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/4/testReport/
Max. process+thread count 2990 (vs. ulimit of 5500)
modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2197/4/console
versions git=2.17.1 maven=3.6.0 findbugs=4.0.6
Powered by Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@liuml07 liuml07 merged commit a932796 into apache:trunk Aug 25, 2020
asfgit pushed a commit that referenced this pull request Aug 25, 2020
…ss (#2197)

Contributed by Sandeep Guggilam.

Signed-off-by: Mingliang Liu <[email protected]>
Signed-off-by: Steve Loughran <[email protected]>
asfgit pushed a commit that referenced this pull request Aug 25, 2020
…ss (#2197)

Contributed by Sandeep Guggilam.

Signed-off-by: Mingliang Liu <[email protected]>
Signed-off-by: Steve Loughran <[email protected]>
asfgit pushed a commit that referenced this pull request Aug 25, 2020
…ss (#2197)

Contributed by Sandeep Guggilam.

Signed-off-by: Mingliang Liu <[email protected]>
Signed-off-by: Steve Loughran <[email protected]>
@liuml07
Copy link
Member

liuml07 commented Aug 26, 2020

@sguggilam When I backport the code, I went through the code again. I think I found one bug. Before I revert the change, I want to confirm with you. The existing calls of reloginFromKeytab(false) or reloginFromKeytab(true), the parameter value false or true was previously for checkTGT. But with this change, they are for ignoreTimeElapsed. So the code calling the previously private method is broken - I found two places.

So, first we can change the new public API from

  public void reloginFromKeytab(boolean ignoreTimeElapsed) throws IOException {
    reloginFromKeytab(false, ignoreTimeElapsed);
  }

to something like:

  public void reloginFromKeytab(boolean checkTGT) throws IOException {
    reloginFromKeytab(checkTGT, true);
  }

Second, we should replace existing calls reloginFromKeytab(false) or reloginFromKeytab(true) in UGI to reloginFromKeytab(false, false) or reloginFromKeytab(true, false).

Could you explain more? Thanks!

@sguggilam
Copy link
Contributor Author

@liuml07 Yes that's right. How about just adding the existing method reloginFromKeytab(boolean checkTGT) which would just call reloginFromKeytab(checkTGT,false). This way it retains all existing calls. Any new calls who want to force relogin irrespective of anything can just call reloginFromKeytab(ignoreTimeElapsed) which would also set checkTGT to false as the user want to force re-login anyways

Thoughts ?
I can send a quick follow up PR if this is fine

@liuml07
Copy link
Member

liuml07 commented Aug 26, 2020

@sguggilam

  1. We both agree on that we need to change both two callsites in UGI.
  2. For the new API, if we keep it as it, is a bit confusing when user calls with false value when user does not need to ignore last login time. In that case, we still ignore the TGT check since the first parameter is always false.
  public void reloginFromKeytab(boolean ignoreTimeElapsed) throws IOException {
    reloginFromKeytab(false, ignoreTimeElapsed);
  }

Since the forceful login is the main target, how about we create a new method name, and use false for both parameter?

public void forceReloginFromKeytab() throws IOException {
    reloginFromKeytab(false, true);
}
  1. I think since this a bug, it's better not to create followup JIRAs but instead create a new full pull request. So when people backport or check the change for debugging, they do not need to jump different PRs to understand what's happening as addendum. I will revert the commit in git repo, and the new PR will be including the full code. How about that?

@sguggilam
Copy link
Contributor Author

#2 Since the forceful login is the main target, how about we create a new method name, and use false for both parameter?
This makes sense
#3 Sure @liuml07 , I can send out a new PR with the full code. You can revert this

Thanks again for going over the code and catching this

liuml07 added a commit that referenced this pull request Aug 26, 2020
asfgit pushed a commit that referenced this pull request Aug 26, 2020
@liuml07
Copy link
Member

liuml07 commented Aug 26, 2020

@sguggilam

and use false for both parameter?

Yeah, the second parameter actually is true, but you got what I meant 😄 as the above code snippet. I have reverted from trunk branch, and will do that for other branches. Now you can file a new PR to include everything (updated).

Thanks!

@sguggilam
Copy link
Contributor Author

Yes @liuml07 I noticed that but found your code snippet to be right and felt it was a typo. Thanks, will send out a new PR

asfgit pushed a commit that referenced this pull request Aug 26, 2020
asfgit pushed a commit that referenced this pull request Aug 26, 2020
asfgit pushed a commit that referenced this pull request Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants