HBASE-28484: Add ability to replicate to a different tableName #6578
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
eab148
force-pushed
the
HBASE-28484-eboland-impl
branch
from
0ad9568 to
670844c
Compare
eab148
force-pushed
the
HBASE-28484-eboland-impl
branch
from
670844c to
0448f31
Compare
|
@Apache9 Happy New Year! Do you have any thoughts on the new design and implementation? Thank you for all of your help so far! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
added 2 commits
January 9, 2025 09:23
|
🎊 +1 overall
This message was automatically generated. |
|
Will take a look soon. Thanks for preparing the design doc and also the PR. |
|
@Apache9 Would you mind taking a look at this when you get a chance? |
Apache9
reviewed
Jan 25, 2025
| @@ -139,6 +140,8 @@ public ReplicationSink(Configuration conf, RegionServerCoprocessorHost rsServerH | |||
| Class<? extends SourceFSConfigurationProvider> c = | |||
| Class.forName(className).asSubclass(SourceFSConfigurationProvider.class); | |||
| this.provider = c.getDeclaredConstructor().newInstance(); | |||
| } catch (RuntimeException e) { | |||
| @@ -153,6 +156,8 @@ private WALEntrySinkFilter setupWALEntrySinkFilter() throws IOException { | |||
| filter = walEntryFilterClass == null | |||
| ? null | |||
| : (WALEntrySinkFilter) walEntryFilterClass.getDeclaredConstructor().newInstance(); | |||
| } catch (RuntimeException e) { | |||
| @@ -968,6 +968,9 @@ public enum OperationStatusCode { | |||
| public static final String REPLICATION_SINK_SERVICE_CLASSNAME_DEFAULT = | |||
| "org.apache.hadoop.hbase.replication.ReplicationSinkServiceImpl"; | |||
| public static final String REPLICATION_BULKLOAD_ENABLE_KEY = "hbase.replication.bulkload.enabled"; | |||
| public static final String REPLICATION_SINK_TRANSLATOR = "hbase.replication.sink.translator"; | |||
There was a problem hiding this comment.
We'd better not put things in HConstants, put them into the package where we use it?
| import org.apache.hadoop.hbase.TableName; | ||
| import org.apache.yetus.audience.InterfaceAudience; | ||
|
|
||
| @InterfaceAudience.Public |
There was a problem hiding this comment.
This should be IA.LimitedPrivate("CONFIG")? Do we expected users to use it directly in their code?
| Class<?> translatorClass = this.conf.getClass(HConstants.REPLICATION_SINK_TRANSLATOR, | ||
| IdentityReplicationSinkTranslator.class, ReplicationSinkTranslator.class); | ||
| try { | ||
| return (ReplicationSinkTranslator) translatorClass.getDeclaredConstructor().newInstance(); |
There was a problem hiding this comment.
IIRC we have a ReflectionUtils or something to call constructors of a class, here we do not need to pass the Configuration object to it? Maybe the translator needs to load some configurations?
| throw e; | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to instantiate " + translatorClass); | ||
| return new IdentityReplicationSinkTranslator(); |
There was a problem hiding this comment.
Is this the right choice to fallback to default implementation? I'm not sure...
| import org.apache.yetus.audience.InterfaceAudience; | ||
|
|
||
| @InterfaceAudience.Public | ||
| public interface ReplicationSinkTranslator { |
There was a problem hiding this comment.
Better add some javadoc's to explain the meanings of the methods and the usage of this class?
| mutation.setClusterIds(clusterIds); | ||
| mutation.setAttribute(ReplicationUtils.REPLICATION_ATTR_NAME, | ||
| TableName sinkTableName = translator.getSinkTableName(tableName); | ||
| ExtendedCell sinkCell = translator.getSinkExtendedCell(tableName, cell); |
There was a problem hiding this comment.
This is the reason why I want to see the javadoc for this method,as why do we need to pass the original table name in? And I think we will just do tableName mapping, so we do not need to call the above getSinkTableName everytime as all the cells from the WALEntry are for the same table?
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.
Design document
Jira
Currently, replication can only occur if the source and sink clusters both house tables with the same (tableName, family) pairs. This requirement exists so that the sink cluster knows where to persist the data it receives from the source cluster. In this PR, we loosen the naming constraint and give clients more configuration power over the name of their sink namespaces and tableNames.