I just reviewed your PR, Clebert, and made a comment. In general I think it looks great. Nice work.
I've been thinking about your concern, Michael, that the rules on integrations like this aren't clear. I went back and reviewed the mailing list discussion and the discussion on your PR for the Kafka integration using the ConnectorService. As far as I can tell, the main issue with your PR was that many didn't want to house the actual implementation-specific integration code within the Artemis project. It seems to me that this "rule" is being applied consistently in this case as well, namely that Artemis is providing an integration point (i.e. the JDBC layer) but doesn't house implementation-specific code (i.e. Derby). The consensus in our previous discussion was that implementation-specific integration code (e.g. your Kafka connector) should live outside the broker code-base as a separate component with its own release cycle. This is exactly how the integration with Derby is working. Derby lives outside the broker code-base with its own release cycle and is being pulled in via Maven. If your Kafka connector was available via Maven and we wanted to create a broker example that used it I don't think that would be an issue. To be clear, Derby is being packaged with the broker to serve as a default JDBC implementation, but I don't think it would be an issue to package the Kafka connector with the broker if there was a similar, real need. I don't see any contradictions or inequities here. I'd like to confirm a +1 from you before I merge. Let me know what you think. Justin On Mon, Jan 15, 2018 at 1:53 PM, Clebert Suconic <clebert.suco...@gmail.com> wrote: > I am almost done with this.. I should be able to send a PR tomorrow.. > I think it's looking nice. > > On Mon, Jan 15, 2018 at 9:44 AM, Clebert Suconic > <clebert.suco...@gmail.com> wrote: > > @Martyn: That's exactly what I'm planning.. Having the embedded > > Derby.. just for out of box testing. > > > > > > someone would do ./artemis create --jdbc ./server-place > > > > and we would have artemis running with Derby right there. > > > > > > > > Now my question here was... where do we change to add stuff into lib. > > I changed dep.xml but it's not picking it up. > > > > > > > > > > On Mon, Jan 15, 2018 at 7:54 AM, Martyn Taylor <mtay...@redhat.com> > wrote: > >> Michael, > >> > >> I think all Clebert is suggesting here is to have something that works > out > >> the box to demonstrate the JDBC store. Derby is a good candidate as it > can > >> work in memory, and we it in a lot in our tests. I've actually not > talked > >> to Clebert about this, so he can confirm/deny if this was his intent, > but I > >> don't see this being related to maintaining a connector service > >> implementation? The only Derby specific thing here would be to ship the > >> lib as part of the distro and to configure a JDBC URL. I guess we could > >> ask for a JDBC URL as part of the prompt, and tell the user to drop the > lib > >> on the class path, but having a quick and easy way to set up and test > >> Artemis + JDBC sounds like a UX win to me. > >> > >> Cheers > >> > >> > >> > >> > >> > >> > >> > >> On Mon, Jan 15, 2018 at 7:24 AM, Michael André Pearce < > >> michael.andre.pea...@me.com> wrote: > >> > >>> Well it kind of is. > >>> > >>> are we then saying if a third party lib in this case Derby DB > implements > >>> an interface that artemis provides in this case JDBC in the other case > >>> ConnectorService we are happy to depend on it and ship it with artemis? > >>> > >>> I really don’t want to upset or annoy you but at the same time I > really do > >>> want an even playing field. > >>> > >>> As I already said I’m happy for artemis to have these. I think if > someone > >>> is willing to support it let it be there. If it ends up being > unsupportable > >>> remove it. Though that wasn’t the outcome from the last discussion. > >>> > >>> I really do think we need to have clear rules on this. That are generic > >>> about any component, for anyone. > >>> > >>> eg if a component lies without someone maintaining then after 6months > it > >>> goes to inactive, if still after a year no one steps in it gets > removed and > >>> moves to an attic. > >>> > >>> > >>> > >>> Sent from my iPhone > >>> > >>> > On 15 Jan 2018, at 02:14, Clebert Suconic <clebert.suco...@gmail.com > > > >>> wrote: > >>> > > >>> > That’s different. We are not implementing JDBC here. > >>> > > >>> > > >>> > We can still provide a pluggavle interface and have the feature you > wrote > >>> > plugging into artemis. Even adding examples with dependencies > towards > >>> it. > >>> > I think it was agreed back then. > >>> > > >>> > > >>> > That’s a different discussion from here though. > >>> > > >>> > On Sun, Jan 14, 2018 at 5:26 PM Michael André Pearce < > >>> > michael.andre.pea...@me.com> wrote: > >>> > > >>> >> Yes. And JDBC the pluggable interface point, JDBC is the api. This > is > >>> just > >>> >> as ConnectorService is the pluggable interface that’s a feature. > >>> >> > >>> >> Either we have some provided implementations of the pluggable > interfaces > >>> >> that exist within artemis or none at all. > >>> >> > >>> >> I really see this as no different. I’m happy for it to be there, but > >>> then > >>> >> I’d like this to applied in general, and to add the kafka > >>> ConnectorService. > >>> >> > >>> >> > >>> >> > >>> >> Sent from my iPhone > >>> >> > >>> >>> On 14 Jan 2018, at 21:05, Clebert Suconic < > clebert.suco...@gmail.com> > >>> >> wrote: > >>> >>> > >>> >>> We already have jdnc as a feature. > >>> >>> > >>> >>> On Sun, Jan 14, 2018 at 2:47 PM Michael André Pearce < > >>> >>> michael.andre.pea...@me.com> wrote: > >>> >>> > >>> >>>> My two cents is about consistency of either we do provide > integrations > >>> >>>> with other products out the box or not. > >>> >>>> > >>> >>>> If the arguments of people not wanting to add Kafka clients to the > >>> class > >>> >>>> path for ability to link Artemis with Kafka, because it means > having > >>> to > >>> >>>> maintain it (see it’s thread for all discussion), I don’t see why > >>> >> linking > >>> >>>> Artemis with a specific JDBC vendors product like Apache Derby is > any > >>> >>>> different here. > >>> >>>> > >>> >>>> Not that I’m against this, quite the contrary actually if I could > add > >>> >>>> Kafka integration, but I would like this ruling to be consistent. > >>> >>>> > >>> >>>> > >>> >>>> > >>> >>>> Sent from my iPhone > >>> >>>> > >>> >>>>> On 12 Jan 2018, at 23:51, Clebert Suconic < > clebert.suco...@gmail.com > >>> > > >>> >>>> wrote: > >>> >>>>> > >>> >>>>> I would like to add an option on artemis create to enable jdbc. > >>> >>>>> > >>> >>>>> > >>> >>>>> By default (if you don't provide any other configuration) it > will use > >>> >>>>> derby by default on the properties. > >>> >>>>> > >>> >>>>> > >>> >>>>> And I wanted to add derby as a dependency on ./lib > >>> >>>>> > >>> >>>>> > >>> >>>>> Anyone against it? > >>> >>>>> > >>> >>>>> > >>> >>>>> so, you would do ./artemis create --jdbc > >>> >>>>> > >>> >>>>> > >>> >>>>> and it would configure derby as an option > >>> >>>>> > >>> >>>>> > >>> >>>>> > >>> >>>>> (JDBC is not encouraged.. the journal is the best option.. but > same > >>> as > >>> >>>>> in ActiveMQ5, some people need it). > >>> >>>>> > >>> >>>>> > >>> >>>>> > >>> >>>>> > >>> >>>>> also: I'm trying to understand what I need to change on dep.xml > under > >>> >>>>> artemis-distribution, but I can't make it to work... anyone can > give > >>> >>>>> me a hand on that? > >>> >>>>> > >>> >>>>> > >>> >>>>> > >>> >>>>> > >>> >>>>> -- > >>> >>>>> Clebert Suconic > >>> >>>> > >>> >>> -- > >>> >>> Clebert Suconic > >>> >> > >>> > -- > >>> > Clebert Suconic > >>> > > > > > > > > -- > > Clebert Suconic > > > > -- > Clebert Suconic >
