PHOENIX-5565 Unify index update structures in IndexRegionObserver and…#625
PHOENIX-5565 Unify index update structures in IndexRegionObserver and…#625kadirozde wants to merge 1 commit intoapache:masterfrom kadirozde:5565
Conversation
| HTableInterfaceReference hTableInterfaceReference = | ||
| new HTableInterfaceReference(new ImmutableBytesPtr(tableName)); | ||
| List<Pair<Mutation, byte[]>> localIndexUpdates = indexUpdates.removeAll(hTableInterfaceReference); | ||
| if (localIndexUpdates == null) { |
There was a problem hiding this comment.
I think removeAll will return an empty list rather than null. Let's add a size check here as well
There was a problem hiding this comment.
Why do you think so? Please notice the removeAll returns List.
There was a problem hiding this comment.
I misunderstood your comment. I thought you implied that we cannot use removeAll here. I will add the size check although it is not needed for correctness as the rest of the code will not do anything if the list is empty.
phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
Show resolved
Hide resolved
gjacoby126
left a comment
gjacoby126
left a comment
There was a problem hiding this comment.
Looks like a good simplification
| // The collection of index mutations that will be applied before the data table mutations. The empty column (i.e., | ||
| // the verified column) will have the value false ("unverified") on these mutations | ||
| private Collection<Pair<Mutation, byte[]>> preIndexUpdates = Collections.emptyList(); | ||
| private ListMultimap<HTableInterfaceReference, Mutation> preIndexUpdates; |
There was a problem hiding this comment.
Doesn't have to be this patch, but we should consider extracting this ListMultimap<HTableInterfaceReference, Mutation> into a class of some kind with named accessor methods so that code dealing with it can be at a higher level of abstraction / more self-documenting.
There was a problem hiding this comment.
Do we have a jira for this? @gjacoby126 If not, I ll file one.
There was a problem hiding this comment.
There's no JIRA that I know of, @swaroopak. Thanks!
There was a problem hiding this comment.
Yes, there is a JIRA on this. It is https://issues.apache.org/jira/browse/PHOENIX-5339
@swaroopak
| } | ||
| for (Pair<Mutation, byte[]> update : updates) { | ||
| results.add(new Pair<>(update, m.getRow())); | ||
| indexUpdates.put(new HTableInterfaceReference(new ImmutableBytesPtr(update.getSecond())), new Pair<>(update.getFirst(), m.getRow())); |
There was a problem hiding this comment.
nit: wouldn't it be more efficient to reuse HTableInterfaceReference rather than create N of them when we're likely mutating only a small number of tables?
There was a problem hiding this comment.
It is not easy to optimize this as for each data table mutation we get updates for all index tables. If we do not create a separate reference for each update, then we need to maintain a hash or list of references and then need to look up on or search them. This may not be more efficient. Let me know if you had something else to suggest here.
… IndexCommitter
4 participants
… IndexCommitter