You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/07/06 05:26:59 UTC
[GitHub] [phoenix] stoty commented on a change in pull request #820: PHOENIX-5861 Delete index data failed,due to pool closed
stoty commented on a change in pull request #820:
URL: https://github.com/apache/phoenix/pull/820#discussion_r449984779
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/Indexer.java
##########
@@ -800,5 +804,62 @@ public static void enableIndexing(HTableDescriptor desc, Class<? extends IndexBu
properties.put(Indexer.INDEX_BUILDER_CONF_KEY, builder.getName());
desc.addCoprocessor(Indexer.class.getName(), null, priority, properties);
}
+
+ /**
+ * stop indexer when region split, but split failed, and then rollback, open region.
+ * this need init builder and writer,recoveryWriter.
+ *
+ * @param env
+ */
+ private void initBuilderAndWriter(RegionCoprocessorEnvironment env) {
+ // init index build manager
+ try {
+ this.builder = new IndexBuildManager(env);
+ } catch (IOException e) {
+ LOG.warn("failed to new index build manager", e);
+ }
+ //init index writer
+ // Clone the config since it is shared
+ Configuration clonedConfig = PropertiesUtil.cloneConfig(env.getConfiguration());
+ /*
+ * Set the rpc controller factory so that the HTables used by IndexWriter would
+ * set the correct priorities on the remote RPC calls.
+ */
+ clonedConfig.setClass(RpcControllerFactory.CUSTOM_CONTROLLER_CONF_KEY,
+ InterRegionServerIndexRpcControllerFactory.class, RpcControllerFactory.class);
+ // lower the number of rpc retries. We inherit config from HConnectionManager#setServerSideHConnectionRetries,
+ // which by default uses a multiplier of 10. That is too many retries for our synchronous index writes
+ clonedConfig.setInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER,
+ env.getConfiguration().getInt(INDEX_WRITER_RPC_RETRIES_NUMBER,
+ DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER));
+ clonedConfig.setInt(HConstants.HBASE_CLIENT_PAUSE, env.getConfiguration()
+ .getInt(INDEX_WRITER_RPC_PAUSE, DEFAULT_INDEX_WRITER_RPC_PAUSE));
+ DelegateRegionCoprocessorEnvironment indexWriterEnv = new DelegateRegionCoprocessorEnvironment(clonedConfig, env);
+ // setup the actual index writer
+ String serverName = env.getRegionServerServices().getServerName().getServerName();
+ try {
+ this.writer = new IndexWriter(indexWriterEnv, serverName + "-index-writer");
+ } catch (IOException e) {
+ LOG.warn("Failed to init index writer ", e);
Review comment:
Again, why catch here ?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/Indexer.java
##########
@@ -800,5 +804,62 @@ public static void enableIndexing(HTableDescriptor desc, Class<? extends IndexBu
properties.put(Indexer.INDEX_BUILDER_CONF_KEY, builder.getName());
desc.addCoprocessor(Indexer.class.getName(), null, priority, properties);
}
+
+ /**
+ * stop indexer when region split, but split failed, and then rollback, open region.
+ * this need init builder and writer,recoveryWriter.
+ *
+ * @param env
+ */
+ private void initBuilderAndWriter(RegionCoprocessorEnvironment env) {
Review comment:
This functions seems similar to start(). Could you explain why do we need a new function ?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/Indexer.java
##########
@@ -800,5 +804,62 @@ public static void enableIndexing(HTableDescriptor desc, Class<? extends IndexBu
properties.put(Indexer.INDEX_BUILDER_CONF_KEY, builder.getName());
desc.addCoprocessor(Indexer.class.getName(), null, priority, properties);
}
+
+ /**
+ * stop indexer when region split, but split failed, and then rollback, open region.
+ * this need init builder and writer,recoveryWriter.
+ *
+ * @param env
+ */
+ private void initBuilderAndWriter(RegionCoprocessorEnvironment env) {
+ // init index build manager
+ try {
+ this.builder = new IndexBuildManager(env);
+ } catch (IOException e) {
+ LOG.warn("failed to new index build manager", e);
Review comment:
Catching the exception here, and letting the function continue doesn't seem right, as we'll run into problems later.
Why not just let them bubble up, and handle them at the caller ?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/Indexer.java
##########
@@ -800,5 +804,62 @@ public static void enableIndexing(HTableDescriptor desc, Class<? extends IndexBu
properties.put(Indexer.INDEX_BUILDER_CONF_KEY, builder.getName());
desc.addCoprocessor(Indexer.class.getName(), null, priority, properties);
}
+
+ /**
+ * stop indexer when region split, but split failed, and then rollback, open region.
+ * this need init builder and writer,recoveryWriter.
+ *
+ * @param env
+ */
+ private void initBuilderAndWriter(RegionCoprocessorEnvironment env) {
+ // init index build manager
+ try {
+ this.builder = new IndexBuildManager(env);
+ } catch (IOException e) {
+ LOG.warn("failed to new index build manager", e);
+ }
+ //init index writer
+ // Clone the config since it is shared
+ Configuration clonedConfig = PropertiesUtil.cloneConfig(env.getConfiguration());
+ /*
+ * Set the rpc controller factory so that the HTables used by IndexWriter would
+ * set the correct priorities on the remote RPC calls.
+ */
+ clonedConfig.setClass(RpcControllerFactory.CUSTOM_CONTROLLER_CONF_KEY,
+ InterRegionServerIndexRpcControllerFactory.class, RpcControllerFactory.class);
+ // lower the number of rpc retries. We inherit config from HConnectionManager#setServerSideHConnectionRetries,
+ // which by default uses a multiplier of 10. That is too many retries for our synchronous index writes
+ clonedConfig.setInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER,
+ env.getConfiguration().getInt(INDEX_WRITER_RPC_RETRIES_NUMBER,
+ DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER));
+ clonedConfig.setInt(HConstants.HBASE_CLIENT_PAUSE, env.getConfiguration()
+ .getInt(INDEX_WRITER_RPC_PAUSE, DEFAULT_INDEX_WRITER_RPC_PAUSE));
+ DelegateRegionCoprocessorEnvironment indexWriterEnv = new DelegateRegionCoprocessorEnvironment(clonedConfig, env);
+ // setup the actual index writer
+ String serverName = env.getRegionServerServices().getServerName().getServerName();
+ try {
+ this.writer = new IndexWriter(indexWriterEnv, serverName + "-index-writer");
+ } catch (IOException e) {
+ LOG.warn("Failed to init index writer ", e);
+ }
+
+ //init recovery wirter
+ try {
+ // get the specified failure policy. We only ever override it in tests, but we need to do it
+ // here
+ Class<? extends IndexFailurePolicy> policyClass =
+ env.getConfiguration().getClass(INDEX_RECOVERY_FAILURE_POLICY_KEY,
+ StoreFailuresInCachePolicy.class, IndexFailurePolicy.class);
+ IndexFailurePolicy policy =
+ policyClass.getConstructor(PerRegionIndexWriteCache.class).newInstance(failedIndexEdits);
+ LOG.debug("Setting up recovery writter with failure policy: " + policy.getClass());
+ recoveryWriter =
+ new RecoveryIndexWriter(policy, indexWriterEnv, serverName + "-recovery-writer");
+ } catch (Exception e) {
+ LOG.warn("Failed to init recovery writer ", e);
Review comment:
Again, why catch here ?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/Indexer.java
##########
@@ -652,14 +652,18 @@ private void doPostWithExceptions(ObserverContext<RegionCoprocessorEnvironment>
@Override
public void postOpen(final ObserverContext<RegionCoprocessorEnvironment> c) {
- Multimap<HTableInterfaceReference, Mutation> updates = failedIndexEdits.getEdits(c.getEnvironment().getRegion());
-
if (this.disabled) {
super.postOpen(c);
return;
}
+ // if stopped is true, and then init writer
+ if (this.stopped) {
+ initBuilderAndWriter(c.getEnvironment());
Review comment:
This would mean that we always re-enable a stopped indexer.
I would guess that there are some cases when we don't want to this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org