You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by mnpoonia <gi...@git.apache.org> on 2018/08/08 14:27:25 UTC
[GitHub] phoenix pull request #323: PHOENIX-4839 Check for memstore scanners if it is...
GitHub user mnpoonia opened a pull request:
https://github.com/apache/phoenix/pull/323
PHOENIX-4839 Check for memstore scanners if it is null before adding …
…it to scanner list
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mnpoonia/phoenix 4.x-HBase-1.3-PHOENIX-4839
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/323.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #323
----
commit 3e3b352c711f909fe71e40baae4b0cd413c36857
Author: Aman Poonia <am...@...>
Date: 2018-08-08T14:24:12Z
PHOENIX-4839 Check for memstore scanners if it is null before adding it to scanner list
----
---
[GitHub] phoenix pull request #323: PHOENIX-4839 Check for memstore scanners if it is...
Posted by mnpoonia <gi...@git.apache.org>.
Github user mnpoonia commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/323#discussion_r209235284
--- Diff: phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/IndexHalfStoreFileReaderGenerator.java ---
@@ -395,7 +395,10 @@ public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocess
.getReader().getHFileReader().hasMVCCInfo(), readPt));
}
}
- keyValueScanners.addAll(((HStore) store).memstore.getScanners(readPt));
+ List<KeyValueScanner> memstoreScanners = ((HStore) store).memstore.getScanners(readPt);
--- End diff --
@joshelser - Thanks for pointing that out(my bad i didn't get that in first look at the code ). Updated PR, please have a look.
---
[GitHub] phoenix pull request #323: PHOENIX-4839 Check for memstore scanners if it is...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/phoenix/pull/323
---
[GitHub] phoenix pull request #323: PHOENIX-4839 Check for memstore scanners if it is...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/323#discussion_r208711828
--- Diff: phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/IndexHalfStoreFileReaderGenerator.java ---
@@ -395,7 +395,10 @@ public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocess
.getReader().getHFileReader().hasMVCCInfo(), readPt));
}
}
- keyValueScanners.addAll(((HStore) store).memstore.getScanners(readPt));
+ List<KeyValueScanner> memstoreScanners = ((HStore) store).memstore.getScanners(readPt);
+ if(memstoreScanners != null) {
--- End diff --
What is the case that this would ever be null?
---
[GitHub] phoenix pull request #323: PHOENIX-4839 Check for memstore scanners if it is...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/323#discussion_r208713485
--- Diff: phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/IndexHalfStoreFileReaderGenerator.java ---
@@ -395,7 +395,10 @@ public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocess
.getReader().getHFileReader().hasMVCCInfo(), readPt));
}
}
- keyValueScanners.addAll(((HStore) store).memstore.getScanners(readPt));
+ List<KeyValueScanner> memstoreScanners = ((HStore) store).memstore.getScanners(readPt);
--- End diff --
In general, we don't ever want to be casting the interface (`Store`) to the implementation (`HStore`). Is phoenix fundamentally doing something here that requires it to be tied to some specific HStore functionality?
---
[GitHub] phoenix pull request #323: PHOENIX-4839 Check for memstore scanners if it is...
Posted by mnpoonia <gi...@git.apache.org>.
Github user mnpoonia commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/323#discussion_r208812073
--- Diff: phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/IndexHalfStoreFileReaderGenerator.java ---
@@ -395,7 +395,10 @@ public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocess
.getReader().getHFileReader().hasMVCCInfo(), readPt));
}
}
- keyValueScanners.addAll(((HStore) store).memstore.getScanners(readPt));
+ List<KeyValueScanner> memstoreScanners = ((HStore) store).memstore.getScanners(readPt);
+ if(memstoreScanners != null) {
--- End diff --
This is the line where it might return null
https://github.com/apache/hbase/blob/branch-1.3/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java#L637
---
[GitHub] phoenix pull request #323: PHOENIX-4839 Check for memstore scanners if it is...
Posted by mnpoonia <gi...@git.apache.org>.
Github user mnpoonia commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/323#discussion_r208821626
--- Diff: phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/IndexHalfStoreFileReaderGenerator.java ---
@@ -395,7 +395,10 @@ public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocess
.getReader().getHFileReader().hasMVCCInfo(), readPt));
}
}
- keyValueScanners.addAll(((HStore) store).memstore.getScanners(readPt));
+ List<KeyValueScanner> memstoreScanners = ((HStore) store).memstore.getScanners(readPt);
--- End diff --
Actually i didn't want to touch the other part of code to fix this specific issue.
---
[GitHub] phoenix pull request #323: PHOENIX-4839 Check for memstore scanners if it is...
Posted by mnpoonia <gi...@git.apache.org>.
Github user mnpoonia commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/323#discussion_r209157109
--- Diff: phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/IndexHalfStoreFileReaderGenerator.java ---
@@ -395,7 +395,10 @@ public KeyValueScanner preStoreScannerOpen(final ObserverContext<RegionCoprocess
.getReader().getHFileReader().hasMVCCInfo(), readPt));
}
}
- keyValueScanners.addAll(((HStore) store).memstore.getScanners(readPt));
+ List<KeyValueScanner> memstoreScanners = ((HStore) store).memstore.getScanners(readPt);
--- End diff --
@joshelser Looks like we need HStore to access memstore specifically. We can't access the memstore and get its scanners from the store object.
---