HBASE-24974: Provide a flexibility to print only row key and filter for multiple tables in the WALPrettyPrinter#2345
Merged
bharathv merged 3 commits intoapache:masterfrom Sep 9, 2020
Conversation
…or multiple tables in the WALPrettyPrinter
Contributor
Author
|
@bharathv can you review this since you last touched this file? |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
reviewed
Sep 3, 2020
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Overall, looks good, just some minor nits.
One additional thought I have here, on cases where same row had several puts in same wal (either overriding ones or because the row has several cells), output can become very repetitive. Maybe for another jira, we can think of a functionality where user can combine the set of info he would like to be displayed?
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALPrettyPrinter.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
nit: for clarity, shouldn't we name it as tableSet instead of tableList? Or maybe better to use List/ArrayList.
Contributor
There was a problem hiding this comment.
+1, and this should be final too.
Contributor
Author
There was a problem hiding this comment.
I used Set since every WALEntry is going to search in the Set which is optimal for search.
as @virajjasani pointed out in another comment
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALPrettyPrinter.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALPrettyPrinter.java
Outdated
Show resolved
Hide resolved
virajjasani
reviewed
Sep 3, 2020
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Left few comments. Overall looks good.
Comment on lines
160
to
162
Contributor
There was a problem hiding this comment.
This can be replaced with:
Collections.addAll(tableList, tableListWithDelimiter.split(","));
Contributor
There was a problem hiding this comment.
Looks like this efficient search with contains() is the reason behind using Set instead of List.
Btw, (TableName) is redundant cast here.
Contributor
There was a problem hiding this comment.
+1, and this should be final too.
bharathv
reviewed
Sep 3, 2020
Contributor
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
While we are here, does it make sense to make the table filter a regex? (ex: for dumping all entries for tables in a namespace etc).
Contributor
Author
|
@bharathv Initially though of that but regex evaluation for each WALEntry is going to be an expensive operation? On the other hand searching is HashSet is O(1). What do you think? |
Contributor
|
Fine by me. |
Contributor
Author
|
@virajjasani @wchevreuil Addressed your comments and tested it locally. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bharathv
approved these changes
Sep 4, 2020
| long writeTime = key.getWriteTime(); | ||
| // check output filters | ||
| if (table != null && !((TableName) txn.get("table")).toString().equals(table)) { | ||
| if (!tableSet.isEmpty() && |
Contributor
There was a problem hiding this comment.
nit: I think the second condition covers the first one.
Contributor
Author
There was a problem hiding this comment.
I think we need the first one where we don't pass -t param and set is Empty. In that case, the second one will always skip all the tables. @bharathv
Contributor
|
@wchevreuil You have any more comments on this ? |
wchevreuil
approved these changes
Sep 9, 2020
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
+1 (Sorry for the late reply, this fell off my radar somehow)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Provide flexibility to print only row key from WALPrettyPrinter and filter for multiple tables in the WALPrettyPrinter