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

HBASE-23590 : Update maxStoreFileRefCount to maxCompactedStoreFileRef… #950

Closed
wants to merge 1 commit into from
Closed

HBASE-23590 : Update maxStoreFileRefCount to maxCompactedStoreFileRef… #950

wants to merge 1 commit into from

Conversation

virajjasani
Copy link
Contributor

…Count for auto region recovery based on old reader references

@virajjasani
HBASE-23590 : Update maxStoreFileRefCount to maxCompactedStoreFileRef…
c405b4c 
…Count for auto region recovery based on old reader references
@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 2m 0s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 1s No case conflicting files found.
+0 🆗 prototool 0m 0s prototool was not available.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 3 new or modified test files.
_ master Compile Tests _
+0 🆗 mvndep 0m 37s Maven dependency ordering for branch
+1 💚 mvninstall 5m 19s master passed
+1 💚 compile 3m 0s master passed
+1 💚 checkstyle 2m 30s master passed
+0 🆗 refguide 5m 32s branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 4m 36s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 5m 2s master passed
+0 🆗 spotbugs 3m 53s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 24m 13s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 15s Maven dependency ordering for patch
+1 💚 mvninstall 4m 52s the patch passed
+1 💚 compile 3m 1s the patch passed
+1 💚 cc 3m 1s the patch passed
+1 💚 javac 3m 1s the patch passed
+1 💚 checkstyle 2m 36s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 xml 0m 1s The patch has no ill-formed XML file.
+0 🆗 refguide 5m 30s patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect.
+1 💚 shadedjars 4m 37s patch has no errors when building our shaded downstream artifacts.
+1 💚 hadoopcheck 15m 42s Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚 hbaseprotoc 8m 42s the patch passed
+1 💚 javadoc 5m 13s the patch passed
+1 💚 findbugs 25m 26s the patch passed
_ Other Tests _
-1 ❌ unit 228m 42s root in the patch failed.
+1 💚 asflicense 4m 36s The patch does not generate ASF License warnings.
370m 23s
Reason Tests
Failed junit tests hadoop.hbase.snapshot.TestExportSnapshotNoCluster
Subsystem Report/Notes
Docker Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-950/1/artifact/out/Dockerfile
GITHUB PR #950
Optional Tests dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile refguide xml cc hbaseprotoc prototool
uname Linux 0952b0c2c878 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux
Build tool maven
Personality /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-950/out/precommit/personality/provided.sh
git revision master / 17e180e
Default Java 1.8.0_181
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-950/1/artifact/out/branch-site/book.html
refguide https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-950/1/artifact/out/patch-site/book.html
unit https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-950/1/artifact/out/patch-unit-root.txt
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-950/1/testReport/
Max. process+thread count 5345 (vs. ulimit of 10000)
modules C: hbase-protocol-shaded hbase-common hbase-hadoop-compat hbase-hadoop2-compat hbase-protocol hbase-client hbase-server . U: .
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-950/1/console
versions git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered by Apache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

HorizonNet
HorizonNet requested changes Dec 30, 2019
View reviewed changes
Copy link
Contributor

@HorizonNet HorizonNet left a comment

Choose a reason for hiding this comment

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

LGTM overall, but left a comment regarding version compatibility.

hbase-client/src/main/java/org/apache/hadoop/hbase/RegionMetrics.java
* of this region
*/
int getMaxStoreFileRefCount();
int getMaxCompactedStoreFileRefCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

Because RegionMetrics is a public interface, could you please deprecate getMaxStoreFileRefCount and add getMaxCompactedStoreFileRefCount as a new method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okk, yes ideally we would do that but in this case, this change int getMaxStoreFileRefCount(); has not yet landed to any release so far and is available to 2.3.0 and 1.6.0 only, no other release branch. Hence, the plan is to update this method now only before 2.3.0 or 1.6.0 makes it to releases.
Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. This seems like a minor gap in our documentation on how we handle deprecations. Would be good to have a common sense and a documentation for it. @saintstack Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with the suggestion.
This new IA method was introduced as part of Jira HBASE-22460 and HBASE-23213 which are not part of any active release so far. Hence IMO, before 2.3.0 or 1.6.0 goes live, ideally we should be good to make this change, but yes agree to follow standard best practices reg such cases if we have common practice available.
It could also be argued that we can change any Public IA method signatures as long as they are not yet present in any active release.

cc: @apurtell @anoopsjohn

anoopsjohn
anoopsjohn approved these changes Dec 31, 2019
View reviewed changes
Copy link
Contributor

@anoopsjohn anoopsjohn left a comment

Choose a reason for hiding this comment

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

So now we have this max compacted file ref count metric only been sending to HM right? No other metric to HM right regarding the store file ref counts

@virajjasani
Copy link
Contributor Author

So now we have this max compacted file ref count metric only been sending to HM right? No other metric to HM right regarding the store file ref counts

That is correct, this is the only one we are concerned about

@virajjasani
Copy link
Contributor Author

virajjasani commented Dec 31, 2019 •
edited

@HorizonNet should be good to go? Please let me know.
Thanks

@virajjasani virajjasani closed this Jan 1, 2020
@virajjasani virajjasani deleted the HBASE-23590-master branch January 1, 2020 18:36
@JeongDaeKim JeongDaeKim mentioned this pull request Jan 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HorizonNet HorizonNet HorizonNet requested changes

@anoopsjohn anoopsjohn anoopsjohn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@virajjasani @Apache-HBase @HorizonNet @anoopsjohn