-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-18095: Zookeeper-less client connection implementation #781
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.hadoop.hbase; | ||
|
|
||
| import org.apache.yetus.audience.InterfaceAudience; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * Thrown by master when meta region locations are not cached whatever reason. | ||
| * Client is expected to retry when running into this. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class MetaRegionsNotAvailableException extends IOException { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there going to be other information served by this registry beside meta regions? Maybe not right now but do you think this mechanism may be overloaded in the future? It seems generally useful... masters as intermediary between clients (or servers) and what is direct access to zookeeper today. It would be better to not have a checked exception for each bit of information served via this means. Would IOException be sufficient?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I argue against an exception entirely -- why is an empty list or |
||
| private static final long serialVersionUID = (1L << 14) - 1L; | ||
|
|
||
| public MetaRegionsNotAvailableException(String msg) { | ||
| super(msg); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -394,4 +394,7 @@ public Hbck getHbck(ServerName masterServer) throws IOException { | |
| Optional<MetricsConnection> getConnectionMetrics() { | ||
| return metrics; | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| AsyncRegistry getRegistry() { return registry; } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. Fine for master and branch-2, but note a headache here for branch-1 (if a backport is desired). The precursor implementation in branch-1 is ClusterRegistry. AsyncRegistry was a big refactor via HBASE-16835. Not suggesting this needs be different, and some simple substitutions may get you most of the way there. Just want to point this out.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for using an accessor method. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,6 @@ | |
|
|
||
| /** | ||
| * Implementations hold cluster information such as this cluster's id, location of hbase:meta, etc.. | ||
| * All stuffs that may be related to zookeeper at client side are placed here. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 |
||
| * <p> | ||
| * Internal use only. | ||
| */ | ||
|
|
@@ -45,21 +44,11 @@ interface AsyncRegistry extends Closeable { | |
| */ | ||
| CompletableFuture<String> getClusterId(); | ||
|
|
||
| /** | ||
wchevreuil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * Get the number of 'running' regionservers. | ||
| */ | ||
| CompletableFuture<Integer> getCurrentNrHRS(); | ||
|
|
||
| /** | ||
| * Get the address of HMaster. | ||
| */ | ||
| CompletableFuture<ServerName> getMasterAddress(); | ||
|
|
||
| /** | ||
| * Get the info port of HMaster. | ||
| */ | ||
| CompletableFuture<Integer> getMasterInfoPort(); | ||
|
|
||
| /** | ||
| * Closes this instance and releases any system resources associated with it | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ | |
| @InterfaceAudience.Private | ||
| final class AsyncRegistryFactory { | ||
|
|
||
| static final String REGISTRY_IMPL_CONF_KEY = "hbase.client.registry.impl"; | ||
| public static final String REGISTRY_IMPL_CONF_KEY = "hbase.client.registry.impl"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO unless some source code requires this to be public, we can keep this unchanged. For test case, we can directly use "hbase.client.registry.impl". Should be fine?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replied in another comment.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should keep it as a public constant, as per @bharathv explanation on the other comment.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the idea here is the clients will instantiate one type of REGISTRY_IMPL, which contacts masters via RPC, and the masters will instantiate another type of REGISTRY_IMPL that talks to zookeeper directly? How is the distinction managed? Maybe I'll answer my own question upon further review...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same concern here. My understanding now, is that both server and client will rely on same config property, which may seem error prone to me. That's why I mention on my previous comment that maybe we should think on split the connection creation path between server and client based callers?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with the comments. Like @apurtell mentioned in another comment, I will work towards making this the default registry implementation for both clients and internal service connections (except Master which obviously goes to ZK, the source of truth). There is nothing in the implementation that prevents this. It was only done because the HBaseMiniCluster used in tests picks random ports (for running concurrent tests) and the clients don't know before hand what would the correct master port to use in the config. So if you see the pattern in tests, we wait for the mini cluster to be up, get the running master and it's port and then create a new Connection object based on that config. Once I figure out a way to force the mini-cluster to use certain known ports (without affecting the test concurrency ofcourse), we can get rid of the whole custom-config / split-config business. I'm looking into it. Hope it clarifies the intention. |
||
|
|
||
| private AsyncRegistryFactory() { | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,236 @@ | ||
| /** | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, you did the dangling javadoc thing :'( |
||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.hadoop.hbase.client; | ||
|
|
||
| import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_RPC_TIMEOUT; | ||
| import static org.apache.hadoop.hbase.HConstants.HBASE_RPC_TIMEOUT_KEY; | ||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.hbase.HConstants; | ||
| import org.apache.hadoop.hbase.HRegionLocation; | ||
| import org.apache.hadoop.hbase.MasterNotRunningException; | ||
| import org.apache.hadoop.hbase.MetaRegionsNotAvailableException; | ||
| import org.apache.hadoop.hbase.RegionLocations; | ||
| import org.apache.hadoop.hbase.ServerName; | ||
| import org.apache.hadoop.hbase.ipc.HBaseRpcController; | ||
| import org.apache.hadoop.hbase.ipc.RpcClient; | ||
| import org.apache.hadoop.hbase.ipc.RpcClientFactory; | ||
| import org.apache.hadoop.hbase.ipc.RpcControllerFactory; | ||
| import org.apache.hadoop.hbase.security.User; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; | ||
|
|
||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.GetClusterIdRequest; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.GetClusterIdResponse; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.GetMetaLocationsRequest; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.GetMetaLocationsResponse; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.IsActiveRequest; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.IsActiveResponse; | ||
| import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProtos.MasterService; | ||
| import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; | ||
| import org.apache.hbase.thirdparty.com.google.protobuf.ServiceException; | ||
|
|
||
| import org.apache.yetus.audience.InterfaceAudience; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * Fetches the meta information directly from HMaster by making relevant RPCs. HMaster RPC end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: endpoint should be one word. |
||
| * points are looked up via client configuration 'hbase.client.asyncregistry.masteraddrs' as | ||
| * comma separated list of host:port values. Should be set in conjunction with | ||
| * 'hbase.client.registry.impl' set to this class impl. | ||
| * | ||
| * The class does not cache anything. It is the responsibility of the callers to cache and | ||
| * avoid repeated requests. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class HMasterAsyncRegistry implements AsyncRegistry { | ||
| private static final Logger LOG = LoggerFactory.getLogger(HMasterAsyncRegistry.class); | ||
| public static final String CONF_KEY = "hbase.client.asyncregistry.masteraddrs"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This doesn't need "asynchregistry" in the key name. We can anticipate multiple consumers of "hbase.client.master.addrs" (suggestion): HMasterAsyncRegistry, HMasterClusterRegistry (in branch-1), something else...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or even "hbase.master.addrs"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you make this CONF_KEY more specific/ meaningful?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While we're nit-picking, I prefer to see all |
||
| private static final String DEFAULT_HOST_PORT = "localhost:" + HConstants.DEFAULT_MASTER_PORT; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this "localhost" being default value for DEFAULT_HOST_PORT useful?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The combination of a host + port (for tcp and udp anyway) is called a "socket address". How about |
||
|
|
||
| // Parsed list of host and ports for masters from hbase-site.xml | ||
| private final List<ServerName> masterServers; | ||
| final Configuration conf; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be private. |
||
| // RPC client used to talk to the master servers. This uses a stand alone RpcClient instance | ||
| // because AsyncRegistry is created prior to creating a cluster Connection. The client is torn | ||
| // down in close(). | ||
| final RpcClient rpcClient; | ||
| final int rpcTimeout; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these two as well should be |
||
|
|
||
| public HMasterAsyncRegistry(Configuration config) { | ||
| masterServers = new ArrayList<>(); | ||
| conf = config; | ||
| parseHortPorts(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| // Passing the default cluster ID means that the token based authentication does not work for | ||
| // certain client implementations. | ||
| // TODO(bharathv): Figure out a way to fetch the CLUSTER ID using a non authenticated way. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thought from the JIRA discussion was to add a servlet which does not require authentication. So, a separate instance of the infoserver stack, simply serving the cluster ID. A bit heavyweight, but an alternative like a separate instance of the RPC stack with auth requirements disabled serving only one iface/method isn't any less heavy. |
||
| rpcClient = RpcClientFactory.createClient(conf, HConstants.CLUSTER_ID_DEFAULT); | ||
| rpcTimeout = (int) Math.min(Integer.MAX_VALUE, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you do toNanos here, is int good enough?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like |
||
| TimeUnit.MILLISECONDS.toNanos(conf.getLong(HBASE_RPC_TIMEOUT_KEY, | ||
| DEFAULT_HBASE_RPC_TIMEOUT))); | ||
| } | ||
|
|
||
| private void parseHortPorts() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. method name typo
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| String hostPorts = conf.get(CONF_KEY, DEFAULT_HOST_PORT); | ||
| for (String hostPort: hostPorts.split(",")) { | ||
| masterServers.add(ServerName.valueOf(hostPort, ServerName.NON_STARTCODE)); | ||
| } | ||
| Preconditions.checkArgument(!masterServers.isEmpty(), String.format("%s is empty", CONF_KEY)); | ||
| // Randomize so that not every client sends requests in the same order. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good |
||
| Collections.shuffle(masterServers); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this shuffle pseudo random that will return same or diff result from different hosts?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect the "default source of randomness" is fine for our needs.
https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#shuffle-java.util.List- |
||
| } | ||
|
|
||
| /** | ||
| * Util that generates a master stub for a given ServerName. | ||
| */ | ||
| private MasterService.BlockingInterface getMasterStub(ServerName server) throws IOException { | ||
| return MasterService.newBlockingStub( | ||
| rpcClient.createBlockingRpcChannel(server, User.getCurrent(), rpcTimeout)); | ||
| } | ||
|
|
||
| /** | ||
| * Blocking RPC to fetch the meta region locations using one of the masters from the parsed list. | ||
| */ | ||
| private RegionLocations getMetaRegionLocationsHelper() throws MetaRegionsNotAvailableException { | ||
| List<HBaseProtos.RegionLocation> result = null; | ||
| for (ServerName sname: masterServers) { | ||
| try { | ||
| MasterService.BlockingInterface stub = getMasterStub(sname); | ||
| HBaseRpcController rpcController = RpcControllerFactory.instantiate(conf).newController(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: can allocate the rpcController once, outside of the loop. |
||
| GetMetaLocationsResponse resp = stub.getMetaLocations(rpcController, | ||
| GetMetaLocationsRequest.getDefaultInstance()); | ||
| result = resp.getLocationsList(); | ||
| } catch (Exception e) { | ||
| LOG.warn("Error fetch meta locations from master {}. Trying others.", sname, e); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: error fetching
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please downgrade the severity of this log message to I'm also not a fan of logging exceptions that are not thrown. Those stack traces make operators nervous. |
||
| } | ||
| } | ||
| if (result == null || result.isEmpty()) { | ||
| throw new MetaRegionsNotAvailableException(String.format( | ||
| "Meta locations not found. Probed masters: %s", conf.get(CONF_KEY, DEFAULT_HOST_PORT))); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:"Meta location not found"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was meant to be for multiple replicas of meta region.. |
||
| } | ||
| List<HRegionLocation> deserializedResult = new ArrayList<>(); | ||
| result.stream().forEach( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| location -> deserializedResult.add(ProtobufUtil.toRegionLocation(location))); | ||
| return new RegionLocations(deserializedResult); | ||
| } | ||
|
|
||
| /** | ||
| * Picks the first master entry from 'masterHortPorts' to fetch the meta region locations. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, nice catch.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might as well make this an But actually the method definition in the interface has a javadoc, so having one here doesn't add much. |
||
| */ | ||
| @Override | ||
| public CompletableFuture<RegionLocations> getMetaRegionLocation() { | ||
| CompletableFuture<RegionLocations> result = new CompletableFuture<>(); | ||
| CompletableFuture.runAsync(() -> { | ||
| try { | ||
| result.complete(this.getMetaRegionLocationsHelper()); | ||
| } catch (Exception e) { | ||
| result.completeExceptionally(e); | ||
| } | ||
| }); | ||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Blocking RPC to get the cluster ID from the parsed master list. Returns null if no active | ||
| * master found. | ||
| */ | ||
| private String getClusterIdHelper() throws MasterNotRunningException { | ||
| // Loop through all the masters serially. We could be hitting some standby masters which cannot | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's worth asking: Why do we need an active master to tell us what the cluster ID is? All the masters can look at the file in HDFS regardless of role. Standbys aren't special here, they can only serve one cluster, just like the active. If we fix this so any master can respond to this query then it's a small improvement in overall availability.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats a good point. I'm looking into it. |
||
| // process this RPC, so we just skip them. | ||
| for (ServerName sname: masterServers) { | ||
| try { | ||
| MasterService.BlockingInterface stub = getMasterStub(sname); | ||
| HBaseRpcController rpcController = RpcControllerFactory.instantiate(conf).newController(); | ||
| GetClusterIdResponse resp = | ||
| stub.getClusterId(rpcController, GetClusterIdRequest.getDefaultInstance()); | ||
| return resp.getClusterId(); | ||
| } catch (IOException e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so, what's the point separating IOException and ServiceException here? |
||
| LOG.warn("Error fetching cluster ID from master: {}", sname, e); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment re: log level and using |
||
| } catch (ServiceException e) { | ||
| // This is probably a standby master, can be ignored. | ||
| LOG.debug("Error fetching cluster ID from server: {}" , sname, e); | ||
| } | ||
| } | ||
| // If it comes to this point, no active master could be found. | ||
| throw new MasterNotRunningException(String.format( | ||
| "No active master found. Probed masters: %s", conf.get(CONF_KEY, DEFAULT_HOST_PORT))); | ||
| } | ||
|
|
||
| @Override | ||
| public CompletableFuture<String> getClusterId() { | ||
| CompletableFuture<String> result = new CompletableFuture<>(); | ||
| CompletableFuture.runAsync(() -> { | ||
| try { | ||
| result.complete(this.getClusterIdHelper()); | ||
| } catch (Exception e) { | ||
| result.completeExceptionally(e); | ||
| } | ||
| }); | ||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Blocking RPC to get the active master address from the parsed list of master servers. | ||
| */ | ||
| private ServerName getMasterAddressHelper() throws MasterNotRunningException { | ||
| for (ServerName sname: masterServers) { | ||
| try { | ||
| MasterService.BlockingInterface stub = getMasterStub(sname); | ||
| HBaseRpcController rpcController = RpcControllerFactory.instantiate(conf).newController(); | ||
| IsActiveResponse resp = stub.isActive(rpcController, IsActiveRequest.getDefaultInstance()); | ||
| if (resp.getIsMasterActive()) { | ||
| return ServerName.valueOf(sname.getHostname(), sname.getPort(), resp.getStartCode()); | ||
| } | ||
| } catch (Exception e) { | ||
|
|
||
| } | ||
| } | ||
| throw new MasterNotRunningException(String.format("No active master found. Probed masters: %s", | ||
| conf.get(CONF_KEY, DEFAULT_HOST_PORT))); | ||
| } | ||
|
|
||
| /** | ||
| * @return the active master among the configured master addresses in 'masterHortPorts'. | ||
| */ | ||
| @Override | ||
| public CompletableFuture<ServerName> getMasterAddress() { | ||
| CompletableFuture<ServerName> result = new CompletableFuture<>(); | ||
| CompletableFuture.runAsync(() -> { | ||
| try { | ||
| result.complete(this.getMasterAddressHelper()); | ||
| } catch (Exception e) { | ||
| result.completeExceptionally(e); | ||
| } | ||
| }); | ||
| return result; | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| if (rpcClient != null) { | ||
| rpcClient.close(); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3188,6 +3188,7 @@ public static org.apache.hadoop.hbase.client.RegionInfo toRegionInfo(final HBase | |
| } | ||
|
|
||
| public static HBaseProtos.RegionLocation toRegionLocation(HRegionLocation loc) { | ||
| if (loc == null) return null; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just curious, did you face NPE conditions while testing this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, but I think it could be null with my patch in error cases where HMaster is not accessible (referring to MetaRegionLocationCache#updateMetaLocation()).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this is a code smell. Better the caller should filter This method is also called by |
||
| HBaseProtos.RegionLocation.Builder builder = HBaseProtos.RegionLocation.newBuilder(); | ||
| builder.setRegionInfo(toRegionInfo(loc.getRegion())); | ||
| if (loc.getServerName() != null) { | ||
|
|
||
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.
Thank you for the single
*and not giving us more "dangling javadoc" warnings 🥇