Hi Jong Wook. Thanks for casting this issue and working on it. I took some time today, to clear the custom patches and find out followings.
1. GetRequest.setMaxResultsPerColumnFamily/setRowOffsetPerColumnFamily methods. I think we do not actually need this method which only exist on our custom patch. Instead of using custom method on GetRequest, I think we can use FilterList. Please check out this commit. https://github.com/apache/incubator-s2graph/compare/master...SteamShon:asynchbase_path?expand=1 2. Scanner.setTimeout. Please note that in https://github.com/apache/incubator-s2graph/compare/master...SteamShon:asynchbase_path?expand=1, I commented out scanner.setTimeout. Current upstream Asynchbase does not allow user to set timeout on scanner. I am going to create issue and send following commits as a PR to upstream. Please check out https://github.com/OpenTSDB/asynchbase/compare/master...SteamShon:scanner_timeout?expand=1 In my humble opinion, while we are waiting on issue on 2 discussed, merged, and finally released by asynchbase guys, I think it is reasonable to use your suggested patch for our first release. On Wed, Sep 28, 2016 at 6:25 AM Jong Wook Kim <[email protected]> wrote: > I managed to created a version using ASM and ByteBuddy to do the > patchwork: > https://github.com/jongwook/incubator-s2graph/commit/770917e0c05228ba0855c16e417436bcc67ff412 > < > https://github.com/jongwook/incubator-s2graph/commit/770917e0c05228ba0855c16e417436bcc67ff412>. > It does: > > - Use ASM to remove the final modifiers of GetRequest and Scanner, and > make all of their methods public > - Dynamically create a subclass of GetRequest, that supports the > limit/offset by adding the bean properties and intercepting serialize() > method. > - Dynamically create a subclass of Scanner, that adds a bean property “int > rpcTimeout” and sets its value in the intercepted getNextRowsRequest(). > > Now the modifications are only about 300 lines as opposed to the previous > ones thousands. > > JW > > > > On Sep 27, 2016, at 3:07 AM, Jong Wook Kim <[email protected]> wrote: > > > > That would be ideal, but asynchbase is being developed according to > OpenTSDB’s own schedules that we don’t have control over. > > > > They don’t release snapshot artifacts and we needed to make a custom > artifact anyway, and given the fact that their releases are usually only > once a year I’m not sure if such PR can be merged in the foreseeable future. > > > > So this patch (as well as the custom artifact and all that jazz) is a > temporary measure until there is the next Asynchbase release that contains > the features we need. > > > > I remember that Doyoung made such PR sometime in the past, but I > couldn’t locate it now. > > > > > > JW > > > >> On Sep 27, 2016, at 2:57 AM, Sergio Fernández <[email protected]> > wrote: > >> > >> As far as I understood, such changes affect mainly configuration > (timeouts, > >> limits, offsets, etc), right? > >> Would not be better to submit a PR to the upstream project to allow to > >> customize such configurations without modifying the code? > >> > >> On Tue, Sep 27, 2016 at 8:49 AM, Jong Wook Kim <[email protected]> > wrote: > >> > >>> Related to the recent comments raised in the vote thread, I’d like to > >>> revisit the asynchbase issue. > >>> > >>> Now their fixes on NSRE are tagged in the recently fixed 1.7.2 on Maven > >>> Central, the remaining differences between our custom version and the > >>> official version are: > >>> > >>> - RPC-wise timeout setting in Scanner > >>> - limit and offset setting in GetRequest > >>> > >>> I made a small patch, as seen in here <https://github.com/jongwook/ > >>> incubator-s2graph/commit/ad5c7f89e46ddbd5dfd9b8721737aa22f94b4002>, > which > >>> includes GetRequest.java and Scanner.java in the s2core tree along > with a > >>> utility that forces loading the bytecode from s2core’s classpath. > >>> > >>> Having two ~1000-line java files which are duplicates might be a bad > >>> practice, but it eliminates the need to maintain a separate codebase > and > >>> maven repository for those small patches. > >>> > >>> To be more aesthetically satisfying, I’ve fiddled a little bit with > >>> bytebuddy <http://bytebuddy.net/#/> to make runtime modification of > the > >>> behaviors of those classes, but the modifications are kindof scattered > >>> making it harder to write the dynamic proxy. > >>> > >>> I’d appreciate any comments on the patch above. > >>> > >>> JW > >> > >> > >> > >> > >> -- > >> Sergio Fernández > >> Partner Technology Manager > >> Redlink GmbH > >> m: +43 6602747925 > >> e: [email protected] > >> w: http://redlink.co > > > >
