HBASE-23935 : Backport HBASE-22978(with sub-tasks) to branch-1#2312
Merged
virajjasani merged 7 commits intoapache:branch-1from Oct 16, 2020
Merged
HBASE-23935 : Backport HBASE-22978(with sub-tasks) to branch-1#2312virajjasani merged 7 commits intoapache:branch-1from
virajjasani merged 7 commits intoapache:branch-1from
Conversation
|
💔 -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. |
virajjasani
force-pushed
the
HBASE-23935-branch-1
branch
from
63fdd36 to
da2f8a8
Compare
|
💔 -1 overall
This message was automatically generated. |
virajjasani
force-pushed
the
HBASE-23935-branch-1
branch
from
da2f8a8 to
da4ca86
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
force-pushed
the
HBASE-23935-branch-1
branch
from
81e87d4 to
f596303
Compare
|
💔 -1 overall
This message was automatically generated. |
virajjasani
force-pushed
the
HBASE-23935-branch-1
branch
from
16b3ba4 to
c297084
Compare
Contributor
Author
|
checkstyle issues belong to part of the code that is not updated in this patch. |
Contributor
Author
|
Javadoc issue in the latest build is also not relevant. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
force-pushed
the
HBASE-23935-branch-1
branch
from
777a139 to
f9d9969
Compare
virajjasani
force-pushed
the
HBASE-23935-branch-1
branch
from
f9d9969 to
b40d94d
Compare
|
💔 -1 overall
This message was automatically generated. |
virajjasani
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
apurtell
requested changes
Sep 23, 2020
Contributor
There was a problem hiding this comment.
First pass looks ok.
Please clean up the commit message. How about
HBASE-23935 Backport HBASE-22978 (with sub-tasks)
- HBASE-XXXXX: foo
- HBASE-XXXXXY: bar
...
?
The javac results from precommit can be ignored.
The checkstyle results from precommit should be cleaned up.
The rubocop results from precommit look like they apply in many cases. Please clean this up. If you need to forward port the changes to keep things consistent we can do that as a followup JIRA. It would also be fine to clean up rubocop issues in shell in general as a followup JIRA.
The xml results from precommit look like a test environment problem and can be ignored.
Unit test failures appear unrelated and can be ignored.
Contributor
Author
checkstyle issue present in the precommit is not relevant to the source code we are updating as part of this patch.
Sure, applying majority of them.
Sure, it makes it easier to identify changes clubbed. |
virajjasani
changed the title
Contributor
Author
|
Updated PR subject (commit message) and PR description (commit description). |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
apurtell
approved these changes
Oct 16, 2020
Contributor
apurtell
left a comment
apurtell
left a comment
There was a problem hiding this comment.
Skimmed the patch. Looked for branch-1 specific concerns like different annotations, logger, and a ban on Java 8-isms. Didn't see any. There was one change that looked suspicious, made a note of it. Please double check.
|
|
||
| double initCost = currentCost; | ||
| double newCost = currentCost; | ||
| double newCost; |
Contributor
There was a problem hiding this comment.
What happened here? Seems like an extraneous change and maybe wrong?
Contributor
Author
There was a problem hiding this comment.
Oh this is anyways a redundant assignment, not an issue. It's not being used anywhere other than for loop and inside that loop, it is always assigned with new value. In fact, this was removed from trunk and branch-2 as well. I am confident this is not problematic at all.
Reidddddd
reviewed
Oct 16, 2020
| Bytes.toBytes(slowLogPayload.getType().name())) | ||
| .addColumn(HConstants.SLOWLOG_INFO_FAMILY, Bytes.toBytes("username"), | ||
| Bytes.toBytes(slowLogPayload.getUserName())); | ||
| puts.add(put); |
Contributor
There was a problem hiding this comment.
although it is back-port. I want to ask, why there's no table name included. From my experience, use region name to search table is very time-spending.
Contributor
Author
There was a problem hiding this comment.
Here, the intention is to just collect list of puts and then carry it over to doPut() method on L116.
Reidddddd
reviewed
Oct 16, 2020
|
|
||
| private static void doPut(final Connection connection, final List<Put> puts) | ||
| throws IOException { | ||
| try (Table table = connection.getTable(SLOW_LOG_TABLE_NAME)) { |
Contributor
There was a problem hiding this comment.
This can be done afterwards.
So here I suggest, we should create something like TTL, if the connections hasn't been touched for a time, there then we close it, otherwise we should keep it.
Contributor
Author
There was a problem hiding this comment.
That's nice idea. Do we already do this as part of some utility? Then I can utilize it here?
Contributor
Author
There was a problem hiding this comment.
@Reidddddd if you are fine, let me take care of this separately i.e search for similar utility or handle it separately here. Otherwise I hope for backport this might not be a concern.
Btw we are doing this (creating connection should be executed once):
try {
if (connection == null) {
createConnection(configuration);
}
doPut(connection, puts);
} catch (Exception e) {
LOG.warn("Failed to add slow/large log records to hbase:slowlog table.", e);
}
Reidddddd
reviewed
Oct 16, 2020
| */ | ||
| public static final int BATCH_ROWS_THRESHOLD_DEFAULT = 5000; | ||
|
|
||
| public static final int DEFAULT_SLOW_LOG_RING_BUFFER_SIZE = 256; |
Contributor
There was a problem hiding this comment.
why do we still introduce these parameters in HConstants. I thought we have avoided to add parameters in it for quite a long time.
Contributor
Author
There was a problem hiding this comment.
That's right, we don't do this anymore. I think the initial patch had multiple references in various classes and hence, we kept all here. e.g SLOW_LOG_BUFFER_ENABLED_KEY is being referred to at 4 places and similarly others too, but since this is a group of constant (usually something that should stay together), they are kept here.
Sounds good?
Reidddddd
approved these changes
Oct 16, 2020
Contributor
Reidddddd
left a comment
Reidddddd
left a comment
There was a problem hiding this comment.
+1 overall, left few comments
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
4 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.
Uh oh!
There was an error while loading. Please reload this page.