Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HADOOP-16256: HTTPFileSystem disallows users from configuring the connectionTimeout and readTimeout of HttpClient #774

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
+61 −0
Open

HADOOP-16256: HTTPFileSystem disallows users from configuring the connectionTimeout and readTimeout of HttpClient #774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions ...project/hadoop-common/src/main/java/org/apache/hadoop/fs/http/AbstractHttpFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.hadoop.fs.http;


import com.google.common.base.Preconditions;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FSDataInputStream;
import org.apache.hadoop.fs.FSDataOutputStream;
Expand All @@ -39,13 +40,19 @@
abstract class AbstractHttpFileSystem extends FileSystem {
private static final long DEFAULT_BLOCK_SIZE = 4096;
private static final Path WORKING_DIR = new Path("/");
private static final String CONF_HTTP_READ_TIMEOUT = "yarn.nodemanager.localizer.http.read.timeout.ms";
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. these should be somewhere where they can be referred to in code, such as a new class HttpFileSystemConstants
  2. And their name must be that of the filesystem class, not anything related to the RM.

private static final String CONF_HTTP_CONNECT_TIMEOUT = "yarn.nodemanager.localizer.http.connect.timeout.ms";
private static final Integer DEFAULT_HTTP_READ_TIMEOUT_IN_MILLIS = 5000;
private static final Integer DEFAULT_HTTP_CONNECT_TIMEOUT_IN_MILLIS = 5000;

private URI uri;
private Configuration conf;

@Override
public void initialize(URI name, Configuration conf) throws IOException {
super.initialize(name, conf);
this.uri = name;
this.conf = conf;
}

public abstract String getScheme();
Expand All @@ -57,7 +64,14 @@ public URI getUri() {

@Override
public FSDataInputStream open(Path path, int bufferSize) throws IOException {
int httpConnectTimeoutInMillis = conf.getInt(CONF_HTTP_CONNECT_TIMEOUT, DEFAULT_HTTP_CONNECT_TIMEOUT_IN_MILLIS);
int httpReadTimeoutInMillis = conf.getInt(CONF_HTTP_READ_TIMEOUT, DEFAULT_HTTP_READ_TIMEOUT_IN_MILLIS);
Preconditions.checkState(httpConnectTimeoutInMillis > 0, "Http connection timeout has to greater than zero.");
Copy link
Contributor

Choose a reason for hiding this comment

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

better:

"Invalid value of " + CONF_HTTP_CONNECT_TIMEOUT +": %d ", httpConnectTimeoutInMillis)

This lists the property name and the actual value set.

Preconditions.checkState(httpReadTimeoutInMillis > 0, "Http read timeout has to greater than zero.");
Copy link
Contributor

Choose a reason for hiding this comment

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

+Same here


URLConnection conn = path.toUri().toURL().openConnection();
conn.setConnectTimeout(httpConnectTimeoutInMillis);
conn.setReadTimeout(httpReadTimeoutInMillis);
InputStream in = conn.getInputStream();
return new FSDataInputStream(new HttpDataInputStream(in));
}
Expand Down
35 changes: 35 additions & 0 deletions ...mon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/http/TestHttpFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.io.IOUtils;
import org.junit.Test;
import org.mockito.Mockito;

import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -64,4 +65,38 @@ public void testHttpFileSystem() throws IOException, URISyntaxException,
assertEquals("/foo", req.getPath());
}
}

@Test(expected = IllegalStateException.class)
public void testHttpFileSystemWithNegativeHttpConnectTimeout() throws Exception {
URI mockURI = new URI("dummyURI");

Configuration conf = new Configuration(false);
conf.set("fs.http.impl", HttpFileSystem.class.getCanonicalName());
conf.set("yarn.nodemanager.localizer.http.connect.timeout.ms", "-1");
Copy link
Contributor

Choose a reason for hiding this comment

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

use the values set in HttpFileSystemConstants

conf.set("yarn.nodemanager.localizer.http.read.timeout.ms", "1000");

Path mockPath = Mockito.mock(Path.class);

HttpFileSystem httpFileSystem = new HttpFileSystem();
httpFileSystem.initialize(mockURI, conf);

httpFileSystem.open(mockPath, 1000);
}

@Test(expected = IllegalStateException.class)
public void testHttpFileSystemWithNegativeHttpReadTimeout() throws Exception {
URI mockURI = new URI("dummyURI");

Configuration conf = new Configuration(false);
conf.set("fs.http.impl", HttpFileSystem.class.getCanonicalName());
conf.set("yarn.nodemanager.localizer.http.connect.timeout.ms", "1000");
conf.set("yarn.nodemanager.localizer.http.read.timeout.ms", "-1");

Path mockPath = Mockito.mock(Path.class);

HttpFileSystem httpFileSystem = new HttpFileSystem();
httpFileSystem.initialize(mockURI, conf);

httpFileSystem.open(mockPath, 1000);
}
}
12 changes: 12 additions & 0 deletions hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1183,6 +1183,18 @@
<value>${yarn.nodemanager.hostname}:8040</value>
</property>

<property>
<description>Http read timeout in milliseconds to use for the localization requests. Setting this configuration is necessary only if you are using either HttpFileSystem or HttpsFileSystem for localization.</description>
<name>yarn.nodemanager.localizer.http.read.timeout.ms</name>
<value>5000</value>
</property>

<property>
<description>Http connect timeout in milliseconds to use for the localization requests. Setting this configuration is necessary only if you are using either HttpFileSystem or HttpsFileSystem for localization.</description>
<name>yarn.nodemanager.localizer.http.connect.timeout.ms</name>
<value>5000</value>
</property>


<property>
<description>Address where the collector service IPC is.</description>
Expand Down