[
https://issues.apache.org/jira/browse/HBASE-19047?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16221380#comment-16221380
]
Appy commented on HBASE-19047:
------------------------------
Really like the parts of patch which is changing args from RegionScanner to
Shipper.
{code}
- private final RegionScanner scanner;
+ private final Shipper shipper;
...
...
- public RegionScannerShippedCallBack(String scannerName, RegionScanner
scanner, Lease lease) {
+ public RegionScannerShippedCallBack(String scannerName, Shipper shipper,
Lease lease) {
{code}
Nice!
----
bq. RegionScanner is exposed to CPs. Right now it extends Shipper means we
expose that too (Even though Shipper is marked as private). We should remove
this and just make the impl class only implement the SHipper interface.
Are we trying to hide Shipper functionality from CPs?
----
{code}
- RegionScanner scanner = null;
if (region.getCoprocessorHost() != null) {
- scanner = region.getCoprocessorHost().preScannerOpen(scan);
- }
- if (scanner == null) {
- scanner = region.getScanner(scan);
+ // preScannerOpen is not allowed to return a RegionScanner. Only post
hook can create a
+ // wrapper for the core created RegionScanner
+ region.getCoprocessorHost().preScannerOpen(scan);
}
+ RegionScannerImpl coreScanner = region.getScanner(scan);
+ Shipper shipper = coreScanner;
+ RegionScanner scanner = coreScanner;
if (region.getCoprocessorHost() != null) {
scanner = region.getCoprocessorHost().postScannerOpen(scan, scanner);
+ if (scanner instanceof Shipper) {
+ shipper = (Shipper) scanner;
+ }
}
{code}
It all made sense until i came to this. Are we trying to hide the Shipper from
CP or not?
If we are trying to hide it, then we shouldn't be doing {{returnedScanner
instanceOf Shipper}} and reassigning shipper.
If we are not trying to hide it, then what we have right now is just fine.
(Actually changing things from RegionScanner *is a* Shipper to RegionScanner
*has a* Shipper might be better - plain old composition over inheritance)
----
{{try (RegionScannerImpl scanner = (RegionScannerImpl) REGION.getScanner(new
Scan())) {}}
Is casting redundant now?
----
> CP exposed Scanner types should not extend Shipper
> --------------------------------------------------
>
> Key: HBASE-19047
> URL: https://issues.apache.org/jira/browse/HBASE-19047
> Project: HBase
> Issue Type: Sub-task
> Components: Coprocessors
> Reporter: Anoop Sam John
> Assignee: Anoop Sam John
> Priority: Critical
> Fix For: 2.0.0-alpha-4
>
> Attachments: HBASE-19047.patch, HBASE-19047_V2.patch,
> HBASE-19047_V2.patch, HBASE-19047_V3.patch, HBASE-19047_V4.patch,
> HBASE-19047_V4.patch, HBASE-19047_V4.patch
>
>
> Shipper is a IA.Private interface and very much internal..
> Right now CP exposed RegionScanner is extending this and so exposing the
> shipped() method. This by mistake is called, can harm the correctness of the
> cells in the Results.
> preScannerOpen() allowing to return a new Scanner is also problematic now.
> This can allow users to create a Region scanner from Region and then wrap it
> and return back (Well same can be done by postScannerOpen also), it can so
> happen that the wrapper is not implementing the shipped() properly. In any
> way exposing the shipped () is problematic.
> Solution Steps
> 1. Remove preScannerOpen() , the use case I can think of is wrapping the
> original scanner. The original scanner can be created by Region.getScanner
> way only.. May be no need to remove this hook. Just remove the ability for
> it to return a RegionScanner instance. Call this with the Scan object and
> the CP can change the Scan object if they want.
> 2. Let RegionScanner not extending Shipper but only RegionScannerImpl
> implements this
> 3. We have ref to the RegionScanner created by core and let that be used by
> RegionScannerShippedCallBack when the post hook doing a wrap.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
