-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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.
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
Upgrading HDFS Repository Plugin to use HDFS 2.8.1 Client #25497
Conversation
WIP primary unit tests is passing, but it's leaking a thread. Gotta check if that's been there before Re-added protobuf fix. Fixed third party library checks. Unit tests fail because of leaked thread. Filter out terrible thread Just update to 2.8.1 while we're here Upgrading the HDFS fixture to 2.8.1 as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please address the comments that I left.
plugins/repository-hdfs/build.gradle
Outdated
|
||
// HDFS | ||
'io.netty.channel.ChannelOption', | ||
'io.netty.util.ReferenceCountUtil' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you sort these, for example, there are already io.netty
excludes above?
* @see "org.apache.hadoop.fs.FileSystem.Statistics.StatisticsDataReferenceCleaner" | ||
* @see "org.apache.hadoop.fs.FileSystem.Statistics" | ||
*/ | ||
public class HdfsClientThreadLeakFilter implements ThreadFilter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be package private and final?
/** | ||
* In Hadoop 2.8.0, there is a thread that is started by the filesystem to clean up old execution stats. | ||
* This thread ignores all interrupts, catching InterruptedException, logging it, and continuing on | ||
* with it's work. The thread is a daemon, so it thankfully does not stop the JVM from closing, and it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it's
-> its
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The only major issue is whether any deps need updating.
|
||
@Override | ||
public boolean reject(Thread t) { | ||
return t.getName().equalsIgnoreCase(OFFENDING_THREAD_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the ignore case needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old habit of mine. I'll fix.
@@ -32,7 +32,7 @@ esplugin { | |||
apply plugin: 'elasticsearch.vagrantsupport' | |||
|
|||
versions << [ | |||
'hadoop2': '2.7.1' | |||
'hadoop2': '2.8.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did no dependency versions change? Just double checking; remember we don't use transitive dependencies, so upgrading means inspecting the new version's deps compared to what we currently pull in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Htrace is the only new dependency with a new version
@@ -83,6 +85,8 @@ public static void main(String[] args) throws Exception { | |||
cfg.set(DFSConfigKeys.DFS_NAMENODE_KEYTAB_FILE_KEY, keytabFile); | |||
cfg.set(DFSConfigKeys.DFS_DATANODE_KEYTAB_FILE_KEY, keytabFile); | |||
cfg.set(DFSConfigKeys.DFS_NAMENODE_ACLS_ENABLED_KEY, "true"); | |||
// dfs.block.access.token.enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this comment line doesn't seem to add anything meaningful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Hadoop 2.7.x libraries fail when running on JDK9 due to the version string changing to a single character. On Hadoop 2.8, this is no longer a problem, and it is unclear on whether the fix will be backported to the 2.7 branch. This commit upgrades our dependency of Hadoop for the HDFS Repository to 2.8.1.
Hadoop 2.7.x libraries fail when running on JDK9 due to the version string changing to a single character. On Hadoop 2.8, this is no longer a problem, and it is unclear on whether the fix will be backported to the 2.7 branch. This commit upgrades our dependency of Hadoop for the HDFS Repository to 2.8.1.
* master: (52 commits) Include shared/attributes.asciidoc from docs master Fixed page breaks for ICU Collation Keyword Fields Remove QueryParseContext (elastic#25486) [Test] Use a common testing class for all XContent filtering tests (elastic#25491) Tests fix - Significant terms/text aggs (elastic#25499) [DOCS] add docs for REST high level client index method (elastic#25501) Tests: Add Debian 9 (Stretch) to the packaging tests test: Run flush before upgrade and refresh after upgrade. Fix third party audit for repository-hdfs [TEST] Expect nodes getting disconnected quickly testPrimaryFailureIncreasesTerm should use assertBusy to wait for yellow Cleanup network / transport related settings (elastic#25489) Fix repository-hdfs plugin packaging test Remove allocation id from replica replication response (elastic#25488) Adjust BWC version on bad allocation request test Upgrading HDFS Repository Plugin to use HDFS 2.8.1 Client (elastic#25497) Adjust status on bad allocation explain requests Preliminary support for ARM Add doc note regarding explicit publish host Fix typo in name of test ...
This should fix #25450. We are upgrading our version of the HDFS client used within the HDFS Repository Plugin in order to fix issues with Hadoop's client code parsing JDK9's version string. The HDFS Test fixture has also been updated to version 2.8.1.