-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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-24401 Cell size limit check on append should consider 0 or less… #1742
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide5.java
Outdated
Show resolved
Hide resolved
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.
Looks good, overall, just had some small nits.
🎊 +1 overall
This message was automatically generated. |
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
+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.
Left a minor point. Does it make sense to move the added test case into its own test?
@@ -2274,6 +2274,21 @@ public void testCellSizeLimit() throws IOException { | |||
// expected | |||
} | |||
} | |||
//test cell size no limit | |||
final TableName noLimitTable = TableName.valueOf("testCellSizeNoLimit"); |
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.
Use the @Rule
provided in this class instead of a String for the table name, such as name.getMethodName()
.
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.
Good suggestion, done.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
} | ||
throw new DoNotRetryIOException(msg); | ||
} else { | ||
LOG.debug("Cell size check is disabled because of maxCellSize is set to {}.", |
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.
Don't need to log this everytime? Only log once is enough when regionserver start.
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.
This parameter can be configured to the column family of the table, different tables can have different values.
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.
ok. I thought this log is not needed. If log this everytime, there are too many logs.
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.
This log may be not very helpful, @wchevreuil what do you think ?
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.
I removed this log, because there may be too many logs and not very helpful.
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.
I removed this log, because there may be too many logs and not very helpful.
Well, that's why I suggested it to be DEBUG. Logging it only once may not be sufficient in cases where log rolls too quickly.
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.
Well, that's why I suggested it to be DEBUG. Logging it only once may not be sufficient in cases where log rolls too quickly.
We could check it in the regionserver configuration and table desc. It ’s OK to remove this log?
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.
Many users and even experienced developers may not be aware of this changed behaviour. When things don't seem to work as expected, and one doesn't have an idea about why, decreasing the log level to gather as much info as possible is a common practice. I know it would be verbose, but that's why we should make it either a DEBUG or a TRACE level message. The message then would give a clear clue for those who didn't even know about the config property or table attribute.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
throw new DoNotRetryIOException(msg); | ||
} | ||
} | ||
} else { | ||
LOG.debug("Cell size check is disabled because of maxCellSize is set to {}.", r.maxCellSize); |
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.
Ditto.
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.
Removed this log.
or less value to disable the check
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
+1
… value to disable the check (#1742) Signed-off-by: Guanghao Zhang <zghao@apache.org>
… value to disable the check (#1742) Signed-off-by: Guanghao Zhang <zghao@apache.org>
Thanks for the reviews all. |
… value to disable the check (#1742) Signed-off-by: Guanghao Zhang <zghao@apache.org>
… value to disable the check (apache#1742) Signed-off-by: Guanghao Zhang <zghao@apache.org>
… value to disable the check (apache#1742) Signed-off-by: Guanghao Zhang <zghao@apache.org>
… value to disable the check (apache#1742) Signed-off-by: Guanghao Zhang <zghao@apache.org> (cherry picked from commit 3cd1525) Change-Id: I08c6a619621107ee5b1c996c3ee5971b968f333e
… value to disable the check