You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/07/12 02:51:17 UTC
[GitHub] [camel] pgalbraith opened a new pull request #5824: camel-websocket: move endpoint init logic into endpoint constructor
pgalbraith opened a new pull request #5824:
URL: https://github.com/apache/camel/pull/5824
Moving the endpoint initialization code from the component into the endpoint constructor makes it considerably easier to extend WebsocketEndpoint with your own customized endpoint.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] pgalbraith commented on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
pgalbraith commented on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-883773202
Hey @oscerd I'd definnitely agree with that concern, but if you look at the endpoint constructor in the original commit, https://github.com/apache/camel/pull/5824/commits/e702a35958d833791acd7878e70b4f96649ea4a6#diff-a48e9686a61008bb4f518d2f32f50dd73b622e9bc2d912c36fe7f69c0163dcdc, I don't think there's any component data being changed, this is only changing endpoint parameters. It's definitely not doing it in the cleanest way possible but I tried to preserve (i.e. cut/paste :-o) as much of the original code from the component createEndpoint as I could, to reduce any possibility of refactoring mistakes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] oscerd commented on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
oscerd commented on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-882262306
I meant to say that, there are options only set at component level and to me they should be maintained in the component class and managed there, the others could be manipulated also on the endpoint class.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus merged pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
davsclaus merged pull request #5824:
URL: https://github.com/apache/camel/pull/5824
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] pgalbraith commented on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
pgalbraith commented on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-882214403
@oscerd what do you think of this alternative?
It's clearly less code so cleaner from that perspective. I prefer the original commit, though, because:
1. This introduces a new protected API method that needs to be supported going forwards
2. If you extend the WebSocketendpoint and change parameters in your endpoint constructor, they will be (possibly) overridden by init code in the component createEndpoint method. This is not intuitive behaviour to me ... endpoint is a natural Camel extension point so I would expect that parameters I set in my endpoint constructor will 'stick'.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
davsclaus commented on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-885495871
Yes @oscerd is correct - Camel does *NOT* use constructors to initialize endpoints - all such logic must be done in its lifecycle phase, build, init, start etc. There may be some components doing some of this still (old way) but there are 300+ so a few may have some old code - and this websocket component is old and based on jetty.
The last commit to introduce a protected method with newXXX is okay - we do something like that in jms component for example.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] oscerd commented on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
oscerd commented on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-883897222
Let's wait for feedback from @davsclaus
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] pgalbraith commented on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
pgalbraith commented on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-885981482
@davcalus I think I misunderstood what you were saying; I wouldn't know one way or the other. If the existing PR is good please merge as I can use it to extend, 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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] pdg-chargelab commented on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
pdg-chargelab commented on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-885724443
Thanks guys, this is educational for me. Would it be too ambitious to think about trying to align this component with the new way of doing things?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] pgalbraith edited a comment on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
pgalbraith edited a comment on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-882214403
@oscerd what do you think of this alternative?
It's clearly less code so cleaner from that perspective. I prefer the original commit, though, because:
1. This introduces a new protected API method that needs to be supported going forwards
2. If you extend the WebsocketEndpoint and change parameters in your endpoint constructor, they will be (possibly) overridden by init code in the component createEndpoint method. This is not intuitive behaviour to me ... endpoint is a natural Camel extension point so I would expect that parameters I set in my endpoint constructor will 'stick'.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] pgalbraith edited a comment on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
pgalbraith edited a comment on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-883773202
Hey @oscerd I'd definitely agree with that concern, but if you look at the endpoint constructor in the original commit, https://github.com/apache/camel/pull/5824/commits/e702a35958d833791acd7878e70b4f96649ea4a6#diff-a48e9686a61008bb4f518d2f32f50dd73b622e9bc2d912c36fe7f69c0163dcdc, I don't think there's any component data being changed, this is only changing endpoint parameters. It's definitely not doing it in the cleanest way possible but I tried to preserve (i.e. cut/paste :-o) as much of the original code from the component createEndpoint as I could, to reduce any possibility of refactoring mistakes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] pgalbraith commented on pull request #5824: camel-websocket: move endpoint init logic into endpoint constructor
Posted by GitBox <gi...@apache.org>.
pgalbraith commented on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-882061362
> I don't think it is a good idea to move all the init logic in the endpoint constructor. Those operations needs to be done on the component side.
Ok I'll see if I can find a better way to do 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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] davsclaus commented on pull request #5824: camel-websocket: safe extension of endpoint
Posted by GitBox <gi...@apache.org>.
davsclaus commented on pull request #5824:
URL: https://github.com/apache/camel/pull/5824#issuecomment-885867678
The endpoint constructor takes in a set of standard options like uri, remaning, component etc so it looks okay as it is now. Then the endpoint is further configurered via getter/setter and whatnot, and potential afterProperties and other special interfaces it can extend. Then there are the lifecycle phases with build, init, start that setup the endpoint and load resources, create clients, thread pools or whatnot.
Where do you see current code that this component could do something the "new way" ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org