[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-505608707 I've cherry-pick'd your commits and squashed them to the new `driver-35` branch based on `master`. we can do major refactoring there, so please target such changes at that branch. I will periodically rebase it on master to keep it up to date. Obviously, we'll need to coordinate a bit when that happens. > Where shall we discuss ideas about the new client? The expectation for Apache projects is that all project communication should happen on the dev list, so feel free to send emails there to get major themes of discussion started. If we get into some minutiae or need some quick back and forth discussion we can just probably talk on IM directly and then report our discussion back to the dev list (which might just mean updating a JIRA or commenting on a PR as we have been doing). closing this PR now. thanks This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-504386330 Does that merge commit need to be in this branch? I think you just needed to rebase on `master` right? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-503291567 Glad you are on board with moving this larger change off to 3.5.0. i guess we need to figure out how to move this work to a feature branch. perhaps i just create a "driver-3.5.0" branch from "master" and then you target this PR at that? From there I can just merge i those changes and that branch would be the home for stabilizing the big driver change for 3.5.0? > Perhaps we can work on small localized changes in the existing client to plug the holes while we work on the 3.5.x story? I will file some JIRAs with the tests that repro the bugs and we can tackle them together. +1 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-501677496 There are still a lot of errors in Travis: https://api.travis-ci.org/v3/job/545093810/log.txt I'm returning to my thinking that we should consider making this a change on the `master` branch for 3.5.0. If we did that we could: 1. skip "deprecation" take a full breaking change and just delete the dead code 1. wait for the netty release with your PR allowing both them and us to confirm stability 1. stage up for some bigger changes to better abstract aspects of the `Client` API (which i want to start on soon and will just create conflicts with this PR which will be annoying to resolve), etc. 1. start a feature branch from `master` and merge this PR into it so that we could collaborate better and evolve this big body of change together. I know that you really wanted to see this on the 3.4.x line, but perhaps you might reconsider that position. Thoughts? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-501225252 Travis failed in what appears to be the same fashion i think - here's the full log: https://api.travis-ci.org/v3/job/544523997/log.txt This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-501209200 Travis failed the Gremlin Server build - I restarted the job so let's see what happened, but the job looked like it hung with this error repeated over and over again: ```text [ERROR] io.netty.util.concurrent.DefaultPromise - Failed to submit a listener notification task. Event loop shut down? java.util.concurrent.RejectedExecutionException: event executor terminated at io.netty.util.concurrent.SingleThreadEventExecutor.reject(SingleThreadEventExecutor.java:867) at io.netty.util.concurrent.SingleThreadEventExecutor.offerTask(SingleThreadEventExecutor.java:328) at io.netty.util.concurrent.SingleThreadEventExecutor.addTask(SingleThreadEventExecutor.java:321) at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:778) at io.netty.util.concurrent.DefaultPromise.safeExecute(DefaultPromise.java:768) at io.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:432) at io.netty.util.concurrent.DefaultPromise.setFailure(DefaultPromise.java:112) at io.netty.channel.DefaultChannelPromise.setFailure(DefaultChannelPromise.java:89) at io.netty.channel.AbstractChannelHandlerContext.safeExecute(AbstractChannelHandlerContext.java:1017) at io.netty.channel.AbstractChannelHandlerContext.close(AbstractChannelHandlerContext.java:610) at io.netty.channel.AbstractChannelHandlerContext.close(AbstractChannelHandlerContext.java:465) at io.netty.channel.DefaultChannelPipeline.close(DefaultChannelPipeline.java:1003) at io.netty.channel.AbstractChannel.close(AbstractChannel.java:242) at io.netty.channel.pool.SimpleChannelPool.close(SimpleChannelPool.java:398) at io.netty.channel.pool.FixedChannelPool.access$1301(FixedChannelPool.java:40) at io.netty.channel.pool.FixedChannelPool$6.run(FixedChannelPool.java:480) at io.netty.util.concurrent.GlobalEventExecutor$TaskRunner.run(GlobalEventExecutor.java:248) at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) at java.lang.Thread.run(Thread.java:748) ``` Do you know what that is? > I have added some documentation. Please check and let me know your thoughts about it. lgtm This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-500579931 Are there concerns with https://github.com/netty/netty/pull/9226 ? Is that a problem we introduce as a new exposed risk as a result of this PR or is it something that can happen now with how things work in `tp34`? I think the code changes seems fine from the best I can tell by just scanning through. I had a few nits around the "deprecated" bits of code. If you could just do a quick pass through and add the appropriate javadoc in the form we use, that would be great. For future reference, here's some additional information in our dev docs on the topic: http://tinkerpop.apache.org/docs/current/dev/developer/#_deprecation No need to include the `@see` link everywhere to the JIRA i don't thinkperhaps a strategically placed one somewhere would suffice. I would really like to see the documentation changes to consider it all a "finished" PR though. Specifically I think I'd like to see: 1. User Documentation - delete the deprecated settings from the docs and modify configuration/best practices sections as needed to reflect how the driver works now. 2. User Upgrade Documentation - something to call attention to this change and explains what was done and why (much of that is here in your PR description so you could probably copy/paste a bit) as well as what users need to do on upgrade to be sure everything works well. 3. Provider Upgrade Documentation - document any things that might possibly break as a result of this change (you mentioned "providers have provided a custom channelizer" for example). Starting the process of testing manually now. Assuming I don't run into any problems there, when you push the documentation changes, I think this PR will be basically good to go. Thanks. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-491969136 > I agree with targeting this change for v3.4.3. :+1: > If any providers have provided a custom channelizer by extending AbstractChannelizer, they will have to modify their channelizer implementation. Is there a way we can inform providers of such implementations? You should write an entry in the upgrade documentation: https://github.com/apache/tinkerpop/blob/master/docs/src/upgrade/release-3.4.x.asciidoc > Is there anything else left (from an implementation perspective) that you would like me to change? I will address the documentation/examples about the working of the client in a separate PR. I will be doing some manual tests this week. Since we can't merge this until after we go through our release process, it should be fine to keep pushing changes to this PR (i.e. the documentation updates). I don't really like to break up doc fixes from code changes, if possible. Is there a reason you would like to see it merge ahead of those changes? since we're waiting for 3.4.3 now there is a bit of a delay to merge anyway right? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-491790730 > which, from a users perspective maintains the same behaviour as the old configuration, i.e. executing 16x32 queries in parallel. Thanks for this clarification. I see it now - I agree. > I am afraid the "switch" is going to be a boolean controlling if/else branches across the code. hmm - well, unless there aren't many such if/else situations such a change might just make the whole thing messy, but i can't imagine that we'd get lucky that way. Maybe you don't need to try to do all that. How do you feel about keeping this targeted at 3.4.x but not merging for the release of 3.4.2 next week? If we did that, we would have ~45 days of testing, review, etc for 3.4.3 (release would be middle/end of july given our current two month release cycles which I believe would continue). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-490462460 > I would be happy to jump on a call/chat to help you understand the architecture of the changes better. Let me know if that is required. thanks, but i think i follow what's been done. > Do we have a tentative release timeline for 3.5.0 yet? No, but we're just starting so I imagine it will take at least through summer to get enough "new" stuff in place to release. > I will write another create slate version for 3.5 later. Couple of additional things I want to fix as part of the clean slate version is to unify the client API, better DNS resolution for potential hosts in the cluster, additional load balancing strategies, ability for providers to plug in information in the request etc. wow - sounds good > Adding additional integration/stress/smoke tests. Unless you have some gaps somewhere that you are aware of, I don't think you need to add anything extra just for the sake of "more tests". You didn't remove any of the old tests (except for one and you had reason for doing so) and they still all work so that inspires confidence. > Adding a config parameter to fallback to old code. More on this below toward the end > Provide a document on working of the new code prior to merge which can be reviewed by the community for potential bugs/design deficiency. From a documentation perspective, I think that this PR should include: 1. Fixes to the current reference documentation to: a. remove references to deprecated configuration options that are now "do nothing" b. include details on best practices for configuring the driver given your new changes 2. Write upgrade documentation to help promote the change and why it was necessary, pointing out what will stop working if you upgrade I think that leads me to my problem with merging this to 3.4.x - If someone upgrades from 3.4.1 to 3.4.2 with this change, their configuration options are no longer relevant. They will just be ignored so if the user managed to get their configuration "perfect" under the old model, they now have to change that given this upgrade. Perhaps that's a simple change (maybe??), but it's a change nonetheless. In TinkerPop's rendition of semantic versioning, `z` upgrades in `x.y.z` should, ideally, have limited impact to users. We occasionally allow breaks in behavior (rarely, if ever, in compilation) in these release versions, but typically those breaks in behavior are the result of a bug fix that can't be resolved any other way. So that said, in a perfect world, your change destined fro 3.4.2 could be staged as a new feature that is parallel to what we already had and the "switch" you suggested earlier could do that. Preferably, we would default to the old functionality with an active switch users would select to utilize the new functionality. Perhaps the "switch" would just be a new `Channelizer` implementation? We could mark the current one as deprecated and then remove it completely in 3.5.0? I guess the changes run deeper than just the `Channelizer` so that may be too simple a solutionwhat were you thoughts on what a "switch" would look like? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [tinkerpop] spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel
spmallette commented on issue #1105: TINKERPOP-2205 Change connection management to single request per channel URL: https://github.com/apache/tinkerpop/pull/1105#issuecomment-490078132 Nice pull request. My initial review mostly focused on getting myself familiar with the changes and I only had some high-level feedback/questions to provide. The additional integration tests you added look really good. Performance numbers look acceptable - thanks for taking that step up front - as we are basically getting similar performance with a more simplified configuration/usage model for users. I've mentioned my concerns earlier about trying to push a change of this size/scope into the 3.4.x line. Seeing the actual changes themselves right now didn't do much to relax those concerns even if it is all backward compatible. Note that we plan to open the 3.5.0 line of code in the next week or so (i.e. `master` will be 3.5.0) which I still feel is a better fit for a change of this scope. If we went with 3.5.0, I think you could view this as a more clean slate and just remove all the "dead" code rather than deprecating things. What do you think - can we go with 3.5.0? I will be doing some manual testing and additional review of this PR in the coming days. Thanks again for working on this. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
