HADOOP-13817. Add a finite shell command timeout to ShellBasedUnixGroupsMapping. #161

QwertyManiac · 2016-11-14T12:00:47Z

Tests required log capture so the LOG variable was elevated in visibility, which required changes in a few test methods unrelated to just this change.
Timeout is applied to both, the regular groups and the partial group listing check commands.
Default is left to 0 to not break current behaviour (i.e. to wait forever until groups are resolvable).

…upsMapping.

xiaoyuyao · 2016-11-14T17:50:54Z

...ject/hadoop-common/src/main/java/org/apache/hadoop/security/ShellBasedUnixGroupsMapping.java

+  @Override
+  public void setConf(Configuration conf) {
+    super.setConf(conf);
+    timeout = conf.getLong(


Can we use Configuration#getTimeDuration() instead of Configuration#getLong to make the newly added configuration key easier to use?

Done in added commit.

xiaoyuyao · 2016-11-14T17:53:55Z

...ject/hadoop-common/src/main/java/org/apache/hadoop/security/ShellBasedUnixGroupsMapping.java

      } catch (PartialGroupNameException pge) {
-        LOG.warn("unable to return groups for user " + user, pge);
+        LOG.warn("unable to return groups for user {}", user, pge);
+        return new LinkedList<>();


Can we use flyweight pattern to minimize memory usage for the empty LinkList?

Done in added commit.

xiaoyuyao · 2016-11-14T17:54:48Z

...ject/hadoop-common/src/main/java/org/apache/hadoop/security/ShellBasedUnixGroupsMapping.java

+            Arrays.asList(executor.getExecString()),
+            timeout
+        );
        return new LinkedList<>();


flyweight pattern applies here as well.

Done in added commit.

- Shared the empty group list return objects - Switched to use of getTimeDuration for the introduced config - Changed the property name to reflect seconds (and removed a redundant word

xiaoyuyao · 2016-11-15T18:36:32Z

...-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java

-  public static final String HADOOP_SECURITY_GROUP_SHELL_COMMAND_TIMEOUT =
-      "hadoop.security.groups.shell.groups.command.timeout";
+  public static final String HADOOP_SECURITY_GROUP_SHELL_COMMAND_TIMEOUT_SECS =
+      "hadoop.security.groups.shell.command.timeout.secs";


I think we should keep "hadoop.security.groups.shell.groups.command.timeout". Sorry I was not clear about this when suggesting Configuration#getTimeDuration. It allows admin to specify the time values with suffix like 10s, 1m, 2h. So the ."secs" won't be needed here.

I'd just felt it would be clearer to have '.secs' in it indicating the minimum unit type despite the parser. I've removed that in the latest commit and added some examples into its doc-text in the xml.

xiaoyuyao · 2016-11-15T18:42:52Z

...ject/hadoop-common/src/main/java/org/apache/hadoop/security/ShellBasedUnixGroupsMapping.java

      LoggerFactory.getLogger(ShellBasedUnixGroupsMapping.class);

  private long timeout = 0L;
+  private final List<String> emptyGroupsList = new LinkedList<>();


This can be static.

Done in the latest commit.

- Removed 'secs' in config name and clarified examples in its docs - Made empty groups list static - Added a null guard in setConf for constructors that do not use ReflectionUtils to pass a valid config

jojochuang · 2017-02-16T18:29:33Z

.../hadoop-common/src/test/java/org/apache/hadoop/security/TestShellBasedUnixGroupsMapping.java

+          ShellBasedUnixGroupsMapping.LOG);
+
+  private static final boolean WINDOWS =
+      (Shell.osType == Shell.OSType.OS_TYPE_WIN);


I think you can just use Shell.WINDOWS instead.

Thank you, addressed in new commit.

jojochuang · 2017-02-16T18:31:05Z

...ject/hadoop-common/src/main/java/org/apache/hadoop/security/ShellBasedUnixGroupsMapping.java

+          message +=
+              " because of the command taking longer than " +
+              "the configured timeout: " + timeout + " seconds";
+          throw new PartialGroupNameException(message);


Maybe this line is not needed if it will throw the exception anyway? The only difference is that it will not carry the original exception

Thanks, addressed in new commit. I just felt the timeout exception may look weird, but I've dropped the line so we can be consistent in exposing the exception at all times.

- Also did a few small cleanups - Exposed the under-timeout stream error as a debug log, just in case its useful. Earlier change just hid it permanently

jojochuang · 2017-02-23T08:57:16Z

...ject/hadoop-common/src/main/java/org/apache/hadoop/security/ShellBasedUnixGroupsMapping.java

+            "command '{}' ran longer than the configured timeout limit of " +
+            "{} seconds.",
+            user,
+            Arrays.asList(executor.getExecString()),


Reviewed the patch again, and I think it's almost ready.
Here's one nit regarding the warning message here: it would print something like "Unable to return groups for user 'foobarnonexistinguser' as shell group lookup command '[sleep, 2]'". But this can be confusing as the command is not run with brackets and comas.

I am +1 pending this and Jenkins precommit build. Somehow Jenkins is never run for your patches.

Thank you again @jojochuang. I've added a new change here addressing your comments. I've also uploaded the full patch directly to JIRA to trigger a build test.

- Instead of printing the command list form, print a readable form - Uses Guava's Joiner over Java 8's native StringJoiner to ensure compat. with branch-2

QwertyManiac · 2017-02-24T16:15:38Z

Done via e8694de.

…ith a ProcessorIdGenerator Minor fix. Unit tests were updated and passed. Author: Sean McCauliff <[email protected]> Reviewers: Navina Ramesh <[email protected]> Closes apache#161 from smccauliff/samza-1259

HADOOP-13817. Add a finite shell command timeout to ShellBasedUnixGro…

6707438

…upsMapping.

xiaoyuyao reviewed Nov 14, 2016

View reviewed changes

Changes addressing @xiaoyuyao's review comments

87e53b0

- Shared the empty group list return objects - Switched to use of getTimeDuration for the introduced config - Changed the property name to reflect seconds (and removed a redundant word

xiaoyuyao reviewed Nov 15, 2016

View reviewed changes

Changes addressing followup comments of @xiaoyuyao

28a6bd4

- Removed 'secs' in config name and clarified examples in its docs - Made empty groups list static - Added a null guard in setConf for constructors that do not use ReflectionUtils to pass a valid config

jojochuang reviewed Feb 16, 2017

View reviewed changes

Changes to address @jojochuang's review

a594e6a

- Also did a few small cleanups - Exposed the under-timeout stream error as a debug log, just in case its useful. Earlier change just hid it permanently

jojochuang reviewed Feb 23, 2017

View reviewed changes

Address final review comment from @jojochuang.

ccddde5

- Instead of printing the command list form, print a readable form - Uses Guava's Joiner over Java 8's native StringJoiner to ensure compat. with branch-2

QwertyManiac closed this Feb 24, 2017

hadoop-yetus mentioned this pull request Aug 28, 2025

HADOOP-19343: Manage hadoop-gcp Guava version directly in its pom.xml and mark exclusion in hadoop-tools-dist. #7904

Closed

4 tasks

hadoop-yetus mentioned this pull request Sep 5, 2025

HADOOP-19674. [JDK 17] Add jaxb-runtime #7928

Merged

4 tasks

hadoop-yetus mentioned this pull request Sep 23, 2025

HADOOP-19224 Upgrade esdk to the latest version 3.24.3 #7003

Closed

4 tasks

HADOOP-13817. Add a finite shell command timeout to ShellBasedUnixGroupsMapping. #161

HADOOP-13817. Add a finite shell command timeout to ShellBasedUnixGroupsMapping. #161

Uh oh!

Conversation

QwertyManiac commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QwertyManiac commented Feb 24, 2017

Uh oh!

Uh oh!