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.

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

HBASE-20557 Backport HBASE-17215 to branch-1 #79

Closed
wants to merge 1 commit into from

Conversation

taklwu
Copy link
Contributor

@taklwu taklwu commented Jun 23, 2018

The second backport of HBASE-20555

@taklwu
Copy link
Contributor Author

taklwu commented Jun 23, 2018

please see the original commit 9facfa550f1e7386be3a04d84f7e8013f5002965 of the master branch

ran mvn clean install -DskipTest, executed unit test TestHFileCleaner and executed mvn test with hbase-server successfully (all with java 8), pinged on HBASE-20557

...
[INFO] Running org.apache.hadoop.hbase.mapreduce.TestHRegionPartitioner
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.559 s - in org.apache.hadoop.hbase.mapreduce.TestHRegionPartitioner
[INFO] Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 121.848 s - in org.apache.hadoop.hbase.mapreduce.TestImportExport
[INFO] 
[INFO] Results:
[INFO] 
[WARNING] Tests run: 1837, Failures: 0, Errors: 0, Skipped: 17
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:42 h
[INFO] Finished at: 2018-06-23T18:07:51Z
[INFO] ------------------------------------------------------------------------

@taklwu taklwu force-pushed the HBASE-17215-branch-1 branch 4 times, most recently from 1e7e02e to df24e16 Compare June 25, 2018 17:29
@z-york
Copy link
Contributor

z-york commented Jun 27, 2018

Can you fix the import order checkstyle warning?

@taklwu taklwu force-pushed the HBASE-17215-branch-1 branch from df24e16 to 0ebd155 Compare June 27, 2018 20:27
@taklwu
Copy link
Contributor Author

taklwu commented Jun 28, 2018

I tried to used the hbase_eclipse_formatter.xml and followed the setting in hbase-checkstyle/src/main/resources/hbase/checkstyle.xml, submitted a new patch for reorganizing the imports. However, it's not working as expected and I don't know what is missing from the checkstyle, you can also see those non-changed classes MergeNormalizationPlan and SplitNormalizationPlan is importing correctly as well but failed with the same issue, I'm wondered if I'm hitting a bug or something for checkstyle, I need more time to look into it but if you can point out the problem that would be helpful


  [ERROR] /local/home/wutaklon/workspace-tmp/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/MergeNormalizationPlan.java:27: Wrong order for 'java.io.IOException' import. [ImportOrder]
  [ERROR] /local/home/wutaklon/workspace-tmp/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/master/normalizer/SplitNormalizationPlan.java:27: Wrong order for 'java.io.IOException' import. [ImportOrder]
...
[ERROR] /local/home/wutaklon/workspace-tmp/hbase/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/HFileCleaner.java:33: Wrong order for 'java.io.IOException' import. [ImportOrder]

@taklwu
Copy link
Contributor Author

taklwu commented Jun 28, 2018

I figured it out at the end that checkstyle of the IDE in hbase_eclipse_formatter.xml and checkstyle while running maven-checkstyle-plugin with conf in hbase-checkstyle/src/main/resources/hbase/checkstyle.xml do not match.

with the following the imports order will pass checkstyle in HFileCleaner.java

import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.LinkedBlockingQueue;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileStatus;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.hbase.Stoppable;
import org.apache.hadoop.hbase.classification.InterfaceAudience;
import org.apache.hadoop.hbase.conf.ConfigurationObserver;
import org.apache.hadoop.hbase.io.HFileLink;
import org.apache.hadoop.hbase.regionserver.StoreFileInfo;

So, I'm wondered if we should have an separate JIRA (I will open it and fix it) to fix and sync between these two setting files (with HBASE-19262 and HBASE-19552) still does not work well, so we need to backport it branch-1 and also fix the problem we found here.

For this changes, I personally see the current commit matched what hbase_eclipse_formatter.xml setup, so, it's safe to commit. but if you insist we need to fix it within this PR, please let me know if we should fix it here instead of a new JIRA/PR.

@z-york
Copy link
Contributor

z-york commented Jun 28, 2018

Let's update the order related to the checkstyle.xml for now and open a JIRA to discuss this mismatch.

The second backport of HBASE-20555
@taklwu taklwu force-pushed the HBASE-17215-branch-1 branch from 0ebd155 to 7bda0b8 Compare June 28, 2018 17:34
@taklwu
Copy link
Contributor Author

taklwu commented Jun 28, 2018

sure, I have done that by updated the commit and patch on the HBASE-20557, the latest auto-build showed that the checkstyle has been passed.

Copy link
Contributor

@z-york z-york left a comment

Choose a reason for hiding this comment

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

Confirmed it is the same change.

@@ -17,22 +17,32 @@
*/
package org.apache.hadoop.hbase.master.cleaner;

import com.google.common.annotations.VisibleForTesting;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird that this wants it all in one block... but if checkstyle likes it who am I to disagree? :)

@taklwu
Copy link
Contributor Author

taklwu commented Jul 3, 2018

any update ? do I need to change anything else?

@z-york
Copy link
Contributor

z-york commented Jul 9, 2018

merged. @busbey how can I close the PR? It looks like I don't have the permissions required.

@taklwu
Copy link
Contributor Author

taklwu commented Jul 23, 2018

this has been merged as a52c6bf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants