[
https://issues.apache.org/jira/browse/PHOENIX-2221?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14958076#comment-14958076
]
James Taylor commented on PHOENIX-2221:
---------------------------------------
Thanks for the patch, [~aliciashu]. I agree with [~rajeshbabu]'s feedback.
Here's some additional feedback:
- Make member variables final, have single constructor that passes both
PerRegionIndexWriteCache and IndexFailurePolicy with a not null precondition
check for the delegator (or you'll get an NPE in the catch block of
handleFailure), and don't assume the type of the delegator to be
KillServerOnFailurePolicy (as this is an implementor or the interface):
{code}
+public class ReadableIndexFailurePolicy extends PhoenixIndexFailurePolicy {
+ private static final Log LOG =
LogFactory.getLog(ReadableIndexFailurePolicy.class);
+ private RegionCoprocessorEnvironment env;
+ private KillServerOnFailurePolicy delegate;
+
+ private PerRegionIndexWriteCache cache;
+ /**
+ * @param failedIndexEdits cache to update when we find a failure
+ */
+ public ReadableIndexFailurePolicy(PerRegionIndexWriteCache
failedIndexEdits) {
+ this.cache = failedIndexEdits;
+ }
+
+ public ReadableIndexFailurePolicy() {
+ }
+
+ public ReadableIndexFailurePolicy(KillServerOnFailurePolicy delegate) {
+ this.delegate = delegate;
+ }
+
{code}
- Best to change PhoenixIndexFailurePolicy to a delegator model as well. Just
have it take an IndexFailurePolicy delegator instead of deriving it from
KillServerOnFailurePolicy.
- Don't derive directly from BaseTest, but instead one of it's subclasses.
- In Indexer, just make INDEX_RECOVERY_FAILURE_POLICY_KEY public instead of
creating a new/duplicate one in QueryServices
- I don't understand why any changes are necessary to PhoenixIndexCodec. The
way you'd use this new feature is by setting INDEX_RECOVERY_FAILURE_POLICY_KEY
in the hbase-site.xml, so the existing code would pick that up and use it. The
one change that probably makes sense is to use
IndexFailurePolicy.class.getName() instead of assuming it's a
PhoenixIndexFailurePolicy class.
{code}
public void initialize(RegionCoprocessorEnvironment env) {
this.env = env;
Configuration conf = env.getConfiguration();
- // Install handler that will attempt to disable the index first before
killing the region
- // server
- conf.setIfUnset(IndexWriter.INDEX_FAILURE_POLICY_CONF_KEY,
- PhoenixIndexFailurePolicy.class.getName());
+ String readable =
conf.get(QueryServices.INDEX_RECOVERY_FAILURE_POLICY_KEY);
+ boolean indexReadable = readable != null &&
readable.equals(ReadableIndexFailurePolicy.class.getName());
+ if (indexReadable) {
+ conf.setIfUnset(IndexWriter.INDEX_FAILURE_POLICY_CONF_KEY,
+ ReadableIndexFailurePolicy.class.getName());
+ } else {
+ // Install handler that will attempt to disable the index first
before killing the region
+ // server
+ conf.setIfUnset(IndexWriter.INDEX_FAILURE_POLICY_CONF_KEY,
+ PhoenixIndexFailurePolicy.class.getName());
+ }
{code}
- Don't understand the changes to QueryOptimizer. An index needs to be ACTIVE
in order for it to be used. Why the check for READABLE?
- Why do we need a new READABLE index state at all? I think it makes more sense
to have a BLOCKED state which prevents writes to the data table until the
background task is able to complete (which should put it back into an ACTIVE
state).
I think it's probably a good idea to first iterate on a design in the JIRA
before coding it. It takes much less time to review than a 1000 line patch.
> Option to make data regions not writable when index regions are not available
> -----------------------------------------------------------------------------
>
> Key: PHOENIX-2221
> URL: https://issues.apache.org/jira/browse/PHOENIX-2221
> Project: Phoenix
> Issue Type: Improvement
> Reporter: Devaraj Das
> Assignee: Alicia Ying Shu
> Attachments: PHOENIX-2221-v1.patch, PHOENIX-2221-v2.patch,
> PHOENIX-2221.patch
>
>
> In one usecase, it was deemed better to not accept writes when the index
> regions are unavailable for any reason (as opposed to disabling the index and
> the queries doing bigger data-table scans).
> The idea is that the index regions are kept consistent with the data regions,
> and when a query runs against the index regions, one can be reasonably sure
> that the query ran with the most recent data in the data regions. When the
> index regions are unavailable, the writes to the data table are rejected.
> Read queries off of the index regions would have deterministic performance
> (and on the other hand if the index is disabled, then the read queries would
> have to go to the data regions until the indexes are rebuilt, and the queries
> would suffer).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
