Conversation
…l expected regions in Canary Signed-off-by: Xu Cang <xucang@apache.org>
Signed-off-by: stack <stack@apache.org> (cherry picked from commit 1cb4f68)
…nd nightly. (apache#621) master/branches-2 specific changes: work around yetus overwriting JAVA_HOME in the container with the host JAVA_HOME. Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> (cherry picked from commit 41990ba) (cherry picked from commit bcad0d9)
apache#524) Signed-off-by: stack <stack@apache.org> (cherry picked from commit f6ff970)
Signed-off-by: Peter Somogyi <psomogyi@apache.org>
…il to clean up java.io.OutputStream Signed-off-by: Andrew Purtell <apurtell@apache.org>
…nload for RegionMover. closes apache#635 Signed-off-by: stack <stack@apache.org> (cherry picked from commit ab076b0)
Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>
… regions (apache#637) Signed-off-by: Duo Zhang <zhangduo@apache.org>
…nown server part (apache#634) Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Sean Busbey <busbey@apache.org>
This reverts commit 5ccab83.
… a "if(LOG.isTraceEnabled())" block. Signed-off-by: Peter Somogyi <psomogyi@apache.org> (cherry picked from commit a85c6b4)
Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Michael Stack <stack@apache.org>
… by the name which contain namespace (apache#639) Signed-off-by: stack <stack@apache.org>
Use correct version for extra-enforcer-rules Signed-off-by: Duo Zhang <zhangduo@apache.org>
…pache#556) * HBASE-22941 merge operation returns parent regions in random order store and return the merge parent regions in ascending order remove left over check for exactly two merged regions add unit test * use SortedMap type to emphasise that the Map is sorted. * use regionCount consistently and checkstyle fixes * Delete tests that expect multiregion merges to fail. Signed-off-by: stack <stack@apache.org>
… where CF contains special characters (like # etc.) Signed-off-by: Sakthi<sakthi@apache.org> (cherry picked from commit 49718b8)
…herChore During startup, it's possible that quotas are enabled but the Master has not yet created the hbase:quotas table. Closes apache#559 Signed-off-by: stack <stack@apache.org> Signed-off-by: Josh Elser <elserj@apache.org>
There was a bug in which we would not drop the RegionSizes for a table in a namespace, where the namespace had a quota on it. This allowed a scenario in which recreation of a table inside of a namespace would unintentionally move into violation despite the table being empty. Need to make sure the RegionSizes are dropped on table deletion if there is _any_ quota applying to that table. Signed-off-by: Josh Elser <elserj@apache.org>
…tTokenUtil, and move ClientTokenUtil to hbase-client (apache#649)
Signed-off-by: Guanghao Zhang <zhangguanghao1@xiaomi.com>
…slower (apache#631) Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: stack <stack@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Norbert Kalmar <nkalmar@cloudera.com>
|
💔 -1 overall
This message was automatically generated. |
|
Merged manually after fixing checkstyle. Tried the failed test locally a few times and passes. |
…egion replicas (apache#1001) Signed-off-by: stack <stack@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
| * Utility for completing passed TableState {@link CompletableFuture} <code>future</code> | ||
| * using passed parameters. | ||
| */ | ||
| private static CompletableFuture<Boolean> completeCheckTableState( |
There was a problem hiding this comment.
What is the return value used for?
There was a problem hiding this comment.
Looks like this a holdover. I can make it void.
There was a problem hiding this comment.
No. Not a holdover. The boolean is true if table is in target state. Let me add javadoc.
There was a problem hiding this comment.
The returned boolean is actually used. Boolean is true if state matches passed in expected state. Leaving.
| * A znode maintained by MirroringTableStateManager. | ||
| * MirroringTableStateManager is deprecated to be removed in hbase3. It can also be disabled. | ||
| * Make sure it is enabled if you want to alter hbase:meta table in hbase2. In hbase3, | ||
| * TBD how metatable state will be hosted; likely on active hbase master. |
There was a problem hiding this comment.
I think the state will either be on zk or on hdfs? HMaster itself is state less...
In general, in the current architecture, I think it the state should be placed on zk, of course you could cache it in master and let client go to master to ask for the state.
There was a problem hiding this comment.
So I do not think we should name this as mirrored, it is the original data. The state in master's memory, is a cache, actually.
There was a problem hiding this comment.
The 'mirroring' feature predates this patch. This is an old facility this patch just makes use of. Changing this old features name is outside realm of this work.
You get the bit that edit of meta is a rare, short-lived event and you get the bit that this implementation fails the state back to ENABLED if ever an issue, intentionally, to minimize our running into exotic situations. I don't want a permanently DISABLED state for hbase:meta, at least not at this stage in the game. We can do that later
There was a problem hiding this comment.
Not relevant in new patch.
| thenApply(state -> { | ||
| return state == null || state.equals(ENABLED_META_TABLE_STATE.getState())? | ||
| ENABLED_META_TABLE_STATE: new TableState(TableName.META_TABLE_NAME, state); | ||
| }).exceptionally(e -> { |
There was a problem hiding this comment.
Currently in HBase, usually we will create a new CompletableFuture and use FutureUtils.addListener to complete it. The code in exceptionally are a bit tricky, where we throw a CompletionException...
There was a problem hiding this comment.
Yeah, saw that. Here, if no znode -- probably because the mirroring table state manager is turned off -- then I want the return to be ENABLED (Completed CF). Should I change this?
There was a problem hiding this comment.
Not relevant in new patch
| private static final Logger LOG = LoggerFactory.getLogger(TableStateManager.class); | ||
|
|
||
| /** | ||
| * All table state is kept in hbase:meta except that of hbase:meta itself. |
There was a problem hiding this comment.
In general I do not think this is a good design. We should persist it somewhere, and just cache it in master's memory. And we do not need to disable a table when altering any more? And the solution here just assume that we only have one meta region? So the altering operation can be atomic? What if we have multiple meta regions and we crash in the middle? I think this patch also aims to implement splittable meta in the future? No?
There was a problem hiding this comment.
bq. We should persist it somewhere, and just cache it in master's memory.
I explain why state is intentionally transient for now so table falls-back to ENABLED if problem like Master crash during alter; its one less thing for the operator to decipher and hbck2 navigate starting up a cluster; in other words we fallback to the 'known' state rather than persist DISABLED meta, a new condition that is sure to have holes in it.
bq. And the solution here just assume that we only have one meta region?
All of the code base presumes this, not just this patch (this patch is part work to undo this presumption letting go of hardcoded meta schema).
bq. I think this patch also aims to implement splittable meta in the future? No?
Splittable meta is another project, not this one.
bq. And we do not need to disable a table when altering any more?
This patch makes hbase:meta dynamic. Maybe the above will work. Let me try.
There was a problem hiding this comment.
I know splittable meta is not implemented by this one but I think we should prepare for it? Not adding new difficulty, by introducing more 'there is only one meta region' assumptions...
And I do not think we need to disable a table before altering it. The issue here is named 'Altering meta', not 'disabling meta', so let's try remove the disable/enable stuff and just implement the alter?
There was a problem hiding this comment.
This discussion has been by-passed by your suggestion that we just support alter of meta, and not disable.
| @@ -1,4 +1,6 @@ | |||
| /** | |||
| /* | |||
| * Copyright The Apache Software Foundation | |||
There was a problem hiding this comment.
We've had this discussion before. Its not javadoc so seems right to remove the extra '*'. We should do up an agreement on way to go. Did you say why you think we should preserve the '**'? Is it because its in the formatter?
There was a problem hiding this comment.
I mean the 'Copyright The Apache Software Foundation'.
| setConfiguration(ColumnFamilyDescriptorBuilder.DATA_BLOCK_ENCODING, | ||
| DataBlockEncoding.ROW_INDEX_V1.toString()).build(); | ||
| admin.modifyColumnFamily(TableName.META_TABLE_NAME, cfd); | ||
| admin.addColumnFamily(TableName.META_TABLE_NAME, newCfd); |
There was a problem hiding this comment.
Now we allow users to add/remove families of meta table from client side? A bit dangerous. I think we should only allow a sub set of alter operations for meta table and system tables from client side? At least, we should not let them add or remove a column family, it will introduce very critical problems.
There was a problem hiding this comment.
This is a good point.
There was a problem hiding this comment.
Added blocking of delete to ModifyTableProcedure if hbase:meta and a core column family.
hbase-server/src/main/java/org/apache/hadoop/hbase/TableDescriptors.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * | ||
| * Should be private | ||
| * @deprecated Since 2.3.0. Should be for internal use only. Used by testing. |
There was a problem hiding this comment.
I do not think this should be deprecated? The class is IA.Private so it is OK to put internal methods here.
There was a problem hiding this comment.
You are right. I wanted to take it private/protected but as you note, its Private so I can just do it.
There was a problem hiding this comment.
Leaving it public for now (after removing the deprecated). Need to undo one reference in test before can take it private.
Signed-off-by: Sean Busbey <busbey@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Jan Hentschel <janh@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
|
@Apache9 Thanks for the review. Let me open a follow-on to address helpful feedback (Boolean => Void, Preventing CF deletion, etc.). On the 'bad design', your objections seem a little out-of-place ('... presumes single meta region...') or easy to counter and your worries that this patch unsettles the codebase seem to pale compared to what has already gone into this minor branch. How to proceed? A discussion on dev list? Or would a write up on how this works help? A one-pager? Let me know. Let me reopen this while we are commenting..... |
saintstack
force-pushed
the
HBASE-23055
branch
from
ae45a5f to
cc4e161
Compare
…us" messages" Minor addendum fixing log message.
|
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: stack <stack@apache.org>
This reverts commit d64b0e3.
…e-rest Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: stack <stack@apache.org>
The cleanupBulkLoad method is only called for the first Region in the table which was being bulk loaded into. This means that potentially N-1 other RegionServers (where N is the number of RegionServers) will leak one FileSystem object into the FileSystem cache which will never be cleaned up. We need to do this clean-up as a part of secureBulkLoadHFiles otherwise we cannot guarantee that heap usage won't grow unbounded. Closes apache#1029 Signed-off-by: Sean Busbey <busbey@apache.org>
… any regions its fixing (apache#917) (apache#1037) The current process for an operator, after fixing holes in meta, is to manually disable and enable the whole table. Let's try to avoid bringing the whole table offline if we can. Have the master attempt to queue up assignment procedures for any new regions it creates. Signed-off-by: stack <stack@apache.org>
saintstack
force-pushed
the
HBASE-23055
branch
2 times, most recently
from
4e3f21d to
09b44c8
Compare
Make hbase:meta region schema dynamic. Patch has been under development a good while and its focus has changed a few times so its bloated with fixup from older versions. M hbase-server/src/main/java/org/apache/hadoop/hbase/master/TableStateManager.java M hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZNodePaths.java Shut down access to internals and removed unused methods. M hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java Cleanup/refactor section on replica-handling. M hbase-server/src/main/java/org/apache/hadoop/hbase/util/FSTableDescriptors.java Get hbase:meta schema from filesystem rather than from hard-coding.
saintstack
force-pushed
the
HBASE-23055
branch
from
09b44c8 to
df47dd8
Compare
|
Closing messed up push. |
20 participants
Signed-off-by: Bharath Vissapragada bharathv@apache.org