HBASE-25549 Provide a switch that allows avoiding reopening all regions when modifying a table to prevent RIT storms. #2924
Conversation
This file contains 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
GeorryHuang
force-pushed
the
lazy_alter
branch
from
a30d220
to
e3cfd88
Compare
|
💔 -1 overall
This message was automatically generated. |
GeorryHuang
force-pushed
the
lazy_alter
branch
2 times, most recently
from
0f54edb
to
14ca0e9
Compare
|
💔 -1 overall
This message was automatically generated. |
GeorryHuang
force-pushed
the
lazy_alter
branch
from
14ca0e9
to
9ca71f6
Compare
|
🎊 +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. |
|
Why a new command rather than an optional lazy flag for the existing command? |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
@busbey Actually, I followed up the way of command 'alter_async' do, which added a so-called lazy flag to the existing function 'admin.alter()' of Ruby. The command 'alter_async' also show up in Ruby as a "new command". I guess 'alter_async' followed some guidelines? |
|
@GeorryHuang I added a comment in the jira. IMO its good to roll "async" behavior into the actual alter command implementation. The original commit that added it is a decade old so I'm guessing very few people around here have any context of it. |
GeorryHuang
changed the title
GeorryHuang
force-pushed
the
lazy_alter
branch
4 times, most recently
from
0928fa8
to
b59b96c
Compare
|
🎊 +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. |
GeorryHuang
force-pushed
the
lazy_alter
branch
2 times, most recently
from
4716b7f
to
e27220a
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
gourabtaparia
approved these changes
Oct 16, 2023
|
Sorry for the delay and thanks for working on this @GeorryHuang, let me come back to the review soon as i can. @bbeaudreault if the changes look good to you in the meantime, please feel free to take it forward :) |
wchevreuil
reviewed
Oct 30, 2023
| @@ -72,6 +72,13 @@ def help | |||
| hbase> alter 't1', CONFIGURATION => {'hbase.hregion.scan.loadColumnFamiliesOnDemand' => 'true'} | |||
| hbase> alter 't1', {NAME => 'f2', CONFIGURATION => {'hbase.hstore.blockingStoreFiles' => '10'}} | |||
|
|
|||
| You can also set configuration setting with REOPEN_REGIONS=>'false' to avoid regions RIT, which | |||
There was a problem hiding this comment.
Can we also explain the type of changes that don't allow reopen=false, such as adding/removing CFs or changing table name?
There was a problem hiding this comment.
Sure, I've made the changes
Comment on lines
124
to
125
| 0 != this.unmodifiedTableDescriptor.getTableName() | ||
| .compareTo(this.modifiedTableDescriptor.getTableName()) |
| this(env, newTableDescriptor, latch, oldTableDescriptor, shouldCheckDescriptor, true); | ||
| } | ||
|
|
||
| public ModifyTableProcedure(final MasterProcedureEnv env, |
There was a problem hiding this comment.
since this class is IA.Private, it'd be preferable to not add a new constructor overload unless absolutely necessary. Are there few enough callers that you could simply update one of the existing constructors?
There was a problem hiding this comment.
Removed the new constructor I added before and altered the existing constructor instead.
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java
Show resolved
Hide resolved
|
💔 -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. |
bbeaudreault
reviewed
Nov 1, 2023
| @@ -2763,6 +2763,13 @@ protected String getDescription() { | |||
| private long modifyTable(final TableName tableName, | |||
| final TableDescriptorGetter newDescriptorGetter, final long nonceGroup, final long nonce, | |||
| final boolean shouldCheckDescriptor) throws IOException { | |||
| return modifyTable(tableName, newDescriptorGetter, nonceGroup, nonce, shouldCheckDescriptor, | |||
There was a problem hiding this comment.
I was looking into why we need this overload. I see this is called from addColumn/deleteColumn/modifyColumn. Does it make sense to add reopenRegion support to modifyColumn at least?
There was a problem hiding this comment.
Now we have two methods:
modifyTable(..[five parameters]..)
modifyTable(..[five parameters].., boolean reopenRegions)
The methods 'addColumn/deleteColumn/modifyColumn' that you mentioned will call method modifyTable(..[five parameters]..) directly as before. They are not aware of the 'reopenRegions' parameter.
To make the code structure cleaner, I only changed one line in method modifyTable(..[five parameters]..), which then turns into modifyTable(..[five parameters].., boolean reopenRegions). So now, you can see that modifyTable(..[five parameters]..) calls modifyTable(..[five parameters].., boolean reopenRegions) with reopenRegions=true.
bbeaudreault
approved these changes
Nov 1, 2023
bbeaudreault
approved these changes
Nov 1, 2023
|
💔 -1 overall
This message was automatically generated. |
|
We are good to go? |
virajjasani
approved these changes
Nov 6, 2023
| } | ||
| } | ||
|
|
||
| private boolean isTablePropertyModified(TableDescriptor oldDescriptor, |
There was a problem hiding this comment.
nit: javadoc would be great
Yes, I am going to merge it into the master branch. |
|
💔 -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. |
bbeaudreault
pushed a commit
that referenced
this pull request
Dec 1, 2023
…ns when modifying a table to prevent RIT storms. (#2924) Co-authored-by: Huangzhuoyue <huangzhuoyue@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Esteban Gutierrez <esteban@apache.org> Reviewed-by: Sean Busbey <busbey@apache.org> Reviewed-by: Gourab Taparia
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 an optional lazy_mode for the alter command to modify the TableDescriptor without the region entering the RIT. The modification will take effect when the region is reopened.