HBASE-24459 Move the locateMeta logic from AsyncMetaRegionTableLocato… #2095
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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
asfgit
force-pushed
the
HBASE-11288.splittable-meta
branch
from
db19115 to
ef5b91b
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
reviewed
Jul 19, 2020
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionUtils.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ZKConnectionRegistry.java
Outdated
Show resolved
Hide resolved
Comment on lines
+755
to
+759
| public static CompletableFuture<List<HRegionLocation>> getAllMetaRegionLocations( | ||
| boolean excludeOfflinedSplitParents, | ||
| CompletableFuture<ClientMetaService.Interface> getStubFuture, | ||
| AtomicReference<ClientMetaService.Interface> stubRef, | ||
| RpcControllerFactory rpcControllerFactory, int callTimeoutMs) { |
There was a problem hiding this comment.
Similar to above methods, we can use generics here also:
public static <T extends ClientMetaService.Interface> CompletableFuture<List<HRegionLocation>> getAllMetaRegionLocations(
boolean excludeOfflinedSplitParents, CompletableFuture<T> getStubFuture,
AtomicReference<T> stubRef, RpcControllerFactory rpcControllerFactory, int callTimeoutMs) {
&
public static<T extends ClientMetaService.Interface> void tryClearMasterStubCache(IOException error,
T currentStub, AtomicReference<T> stub) {
Being Utils class, this might suit well, thought?
But if you feel this is overkill, we are good without that change.
There was a problem hiding this comment.
The class is IA.Private so I think we could do this later when we really have the requirement?
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionUtils.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaLocationCache.java
Outdated
Show resolved
Hide resolved
bharathv
reviewed
Jul 20, 2020
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaRegionLocationCache.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistry.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ZKConnectionRegistry.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ZKConnectionRegistry.java
Outdated
Show resolved
Hide resolved
| Function<RpcChannel, T> stubMaker, String type) { | ||
| return getOrFetch(stub, stubMakeFuture, () -> { | ||
| CompletableFuture<T> future = new CompletableFuture<>(); | ||
| addListener(registry.getActiveMaster(), (addr, error) -> { |
There was a problem hiding this comment.
I liked @saintstack's idea in the design doc where we can ask active master for the list of available masters and load balance the RPCs. I think that can be used both here (to randomize the master we are talking to) and in master registry to always maintain a fresh list of masters and only use the initial list of masters a seed input.
(I can quickly add that feature if everyone is okay with it).
There was a problem hiding this comment.
+1. Can file another issue for it.
| try { | ||
| if (cache != null) { | ||
| locs = cache.locateMeta(HConstants.EMPTY_BYTE_ARRAY, RegionLocateType.CURRENT); | ||
| } else { |
There was a problem hiding this comment.
Why do we need the else part? It looks like the cache runs on all masters ?
There was a problem hiding this comment.
The design is when master becomes active, we will serve the requests by the local master region dreictly, so we will set cache to null after we successfully initialize the master local region. This is important as backup masters will also use the getAllMetaRegionLocations method to sync from active master.
There was a problem hiding this comment.
Ah got it, I missed that part in the first iteration, I think you could roll that logic into the cache to keep the lifecycle simple (since the cache already has access to the master and we can check if master.isActive()). That way all the callers only use the cache and we can avoid the logic in finishMasterInitialization logic.
Also as discussed in the design doc, would be nice to have an actual versioned cache that avoids round trips if nothing changes (for the future). The 1s pull seems a bit aggressive but is fine as a stop-gap I guess.
There was a problem hiding this comment.
Yes, will try to do this as a follow on. Maybe need to introduce another method for doing this, as the return value should have a way tell the upper layer there is no change, and also we should not have the excludeOfflineSplitParent option for this method, otherwise the semantice will be confusing.
There was a problem hiding this comment.
And on the cache stuff, at least we need to stop the chore after switching to active master? And for active master, we will not get from cache any more, still using the cache will be a bit confusing.
There was a problem hiding this comment.
Serving from NavigableMap should be faster than fetch from local Region? For this reason would we want to do like @bharathv suggests (can be follow-on)
There was a problem hiding this comment.
What @bharathv mean is we can reuse the cache interface to simpilify the code, not for performance. After switching to active master we should serve the requests with local region, as we do not want to keep a cache at active master which could be stale?
There was a problem hiding this comment.
I was talking performance. Why would a cache on active Region be stale? Why would it not be updated on write the Region?
There was a problem hiding this comment.
Now the cache is running as a side logic besides the core. Making it always sync with the local region will mess up the code when updating local region, especially that the local region is not for storing root. Since we could also set the family to in memory, I do not think the performance will be much better so I do not think it is worth to also introduce a cache for active master. No big gain but the code will be much complicated.
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaLocationCache.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| protected void chore() { | ||
| AsyncClusterConnection conn = master.getAsyncClusterConnection(); |
There was a problem hiding this comment.
nit: conn reference can be cached once
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MetaLocationCache.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. |
|
🎊 +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
reviewed
Jul 20, 2020
| public static final HBaseClassTestRule CLASS_RULE = | ||
| HBaseClassTestRule.forClass(TestMetaFixerNoCluster.class); | ||
|
|
||
| private static Configuration CONF = HBaseConfiguration.create(); |
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMetaLocationCache.java
Outdated
Show resolved
Hide resolved
| import org.junit.Test; | ||
| import org.junit.experimental.categories.Category; | ||
|
|
||
| @Category({ MasterTests.class, MediumTests.class }) |
There was a problem hiding this comment.
Since we are dealing with Mocks only, this can be SmallTests?
There was a problem hiding this comment.
Not sure, we have sleep in the tests.
There was a problem hiding this comment.
Sure, MediumTests is fine since we have sleep.
|
|
||
| private static Configuration CONF = HBaseConfiguration.create(); | ||
|
|
||
| private static ChoreService CHORE_SERVICE; |
There was a problem hiding this comment.
nit: we can assign value right here and keep this static final
|
|
||
| private static ChoreService CHORE_SERVICE; | ||
|
|
||
| private static byte[] SPLIT = Bytes.toBytes("a"); |
| if (conn != null) { | ||
| addListener(conn.getAllMetaRegionLocations(fetchTimeoutMs), (locs, error) -> { | ||
| if (error != null) { | ||
| LOG.warn("Failed to fetch all meta region locations from active master", error); |
There was a problem hiding this comment.
LOG.error might fit well? Also, what if this chore keeps getting error multiple times? That could mean due to some issue with RPC call, backup masters will serve stale data? Should we rather make this a high priority issue and stop backup masters if we get error say 30-60 times in a consecutive manner?
There was a problem hiding this comment.
Can be a follow on I think. Agree that if active master is down for a long time, we should avoid flooding the log file.
There was a problem hiding this comment.
Sure, we can take it up on follow-up Jira.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bharathv
approved these changes
Jul 21, 2020
| try { | ||
| if (cache != null) { | ||
| locs = cache.locateMeta(HConstants.EMPTY_BYTE_ARRAY, RegionLocateType.CURRENT); | ||
| } else { |
There was a problem hiding this comment.
Ah got it, I missed that part in the first iteration, I think you could roll that logic into the cache to keep the lifecycle simple (since the cache already has access to the master and we can check if master.isActive()). That way all the callers only use the cache and we can avoid the logic in finishMasterInitialization logic.
Also as discussed in the design doc, would be nice to have an actual versioned cache that avoids round trips if nothing changes (for the future). The 1s pull seems a bit aggressive but is fine as a stop-gap I guess.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
asfgit
force-pushed
the
HBASE-11288.splittable-meta
branch
from
9cb6635 to
f249b0b
Compare
…r to ConnectionRegistry
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
Aug 25, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Aug 26, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 5, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 8, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 9, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 10, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 11, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 14, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 15, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 17, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 19, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 20, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 22, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 24, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Sep 27, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Oct 13, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Oct 14, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Oct 29, 2020
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
Apache9
added a commit
that referenced
this pull request
Jul 22, 2021
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
Apache9
added a commit
that referenced
this pull request
Jul 26, 2021
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
Apache9
added a commit
that referenced
this pull request
Jul 29, 2021
…r to ConnectionRegistry (#2095) Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
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.
…r to ConnectionRegistry