Community
Participate
Working Groups
We have gone back and forth on the (I)MatchQuery (Interface and base class). We currently have a @deprecated on our first cut of API, which seems a little strange. (I understand how we got there, it just seems strange). I think we should make an explicit decision before M7 to either keep the class (and remove deprecation warning) or move the class internal (possibly with a replacement that works as a post filter).
At this point I think it is a big risk to remove MatchQuery.
removing milestone for now.
This needs to be discussed at the p2 level. Failing to discuss will have much worst implications.
(In reply to comment #3) > This needs to be discussed at the p2 level. Failing to discuss will have much > worst implications. Start a discussion on the p2-dev mailing list. I know we have consumers of MatchQuery that have no interest in moving to p2 ql. I think it will be very bad and disruptive to break these clients.
We have a good solution for consumers that don't want to move to p2 ql. The solution is included in the deprecation note in the Javadoc and so far no complaints about the deprecation has been voiced on the p2-dev mailing list. The way I remember it we did agree that MatchQuery should be deprecated now and then removed by M7 (on the Skype call just when M6 was about to be built). This is also what the Javadoc written just after the meeting states. Here's a copy for reference: @deprecated This class is likely to be removed in 3.6M7. If possible, use one of the predefined queries in {@link QueryUtil} or use the {@link QueryUtil#createMatchQuery(String, Object...)} to create a custom expression based query. If the query cannot be expressed using the p2QL, then use a predefined or custom expression query as a first filter (in worst case, use {@link QueryUtil#createIUAnyQuery()}) and then provide further filtering like so:<pre> for(iter = queryable.query(someExpressionQuery).iterator(); iter.hasNext();) { // do your match here }</pre> There are reasons for not including MatchQuery, most of then expressed in bug 304130. If more arguments are needed, please run the org.eclipse.equinox.p2.tests.ql.PerformanceTest. If you have clients that you are afraid will be disrupted, please have them voice their concerns on the p2-dev list.
(In reply to comment #5) > If you have clients that you are afraid will be disrupted, please have them > voice their concerns on the p2-dev list. The process is started by you posting to the p2-dev mailing list (and equinox-dev) about the: Risks: Who will be broken that is programming to M6 API Benefits: Why is this a must have and is justified in breaking clients that we know depend on this. And we know several clients that do depend on this! John has valid arguments in bug 304130 for keeping MatchQuery. I don't think there is a clear conclusion to that discussion.
IMHO this issue is as much about perception as it is about the technical details of the two approaches. Undoubtedly there are clients who have match queries, and the engineering cost of converting these queries to either p2ql or even a different Java implementation (custom match q
IMHO this issue is as much about perception as it is about the technical details of the two approaches. Undoubtedly there are clients who have match queries, and the engineering cost of converting these queries to either p2ql or even a different Java implementation (custom match query for example) is too high. As I mentioned before, if we remove MatchQuery, we should consider adding a MatchFilter with roughly the same functionality. The only difference is that a MatchFilter is run on the QueryResult. We could even create a helper method on QueryUtil that creates an ALL query, takes the MatchFilter and seeds the Query Result with this filter. Technically this is no different than the approach Thomas has suggested with the post query iterator. The advantage of this approach is that we can provide clients with an *almost* source compatible way to invoke their existing queries. The only breakage would be in how the query is called (instead of passing your match query to the queryable you call QueryUtil.createFilterQuery(MatchFilter) and pass that query to the queryable). We could even detect and optimize this case so items are only visited once.
(In reply to comment #8) > IMHO this issue is as much about perception as it is about the technical > details of the two approaches. Undoubtedly there are clients who have match > queries, and the engineering cost of converting these queries to either p2ql or > even a different Java implementation (custom match query for example) is too > high. > I agree. > As I mentioned before, if we remove MatchQuery, we should consider adding a > MatchFilter with roughly the same functionality. The only difference is that a > MatchFilter is run on the QueryResult. We could even create a helper method on > QueryUtil that creates an ALL query, takes the MatchFilter and seeds the Query > Result with this filter. > > Technically this is no different than the approach Thomas has suggested with > the post query iterator. The advantage of this approach is that we can provide > clients with an *almost* source compatible way to invoke their existing > queries. The only breakage would be in how the query is called (instead of > passing your match query to the queryable you call > QueryUtil.createFilterQuery(MatchFilter) and pass that query to the queryable). > > We could even detect and optimize this case so items are only visited once. I think it is simply to late to talk about adding another API at this point in Helios. We absolutely need to stabilize.
I'll stick out my neck here and say that I'm willing to rewrite all MatchQueries that can be found out there. I think that would be much less effort then any alternative. So please refer anyone who wants a MatchQuery to me. I'll be the automatic conversion routine for them :-)
(In reply to comment #10) > I'll stick out my neck here and say that I'm willing to rewrite all > MatchQueries that can be found out there. I think that would be much less > effort then any alternative. So please refer anyone who wants a MatchQuery to > me. I'll be the automatic conversion routine for them :-) I appreciate the offer, but that is really not the point here. There are consumers of Helios. They are stabilizing their releases in parallel with the Helios release. They have plans that depend on the API freeze in M6. They have well tested (and quite large) set of scenarios. They will be forced to retest and work through any issues related to these changes. It is not about the time it takes to change a few lines of code. It is the overall delay and the impact this will have for our consumers to release on time.
I would vote -1 to removing the IMatchQuery simply because of the complexity of the p2 ql. Bug 304130 comment #0 is a good summary, and in particular, see bug 304130 comment #19 https://bugs.eclipse.org/bugs/show_bug.cgi?id=304130#c19 > We're currently sitting here trying to debug a test failure involving a > failing expression query, and we don't have the first clue about why it's > failing or how to debug this. I wouldn't inflict this complexity on *anyone* > who doesn't have an overwhelming need for it. Simplicity is a good, I'm sure there are RCP clients who would happily accept slower queries in exchange for it. A common complaint I've heard at things like demo camps is that p2 is over-engineered and too complex. There may be good reasons for all of that complexity, but many people don't care about that.
(In reply to comment #12) > Simplicity is a good yes indeed. > A common complaint I've heard at things like demo camps is that > p2 is over-engineered and too complex. That was exactly the impression I got when I first looked at the p2 query mechanisms. I'd argue that most newcomers, comparing what it used to look like, and what it looks like now, would perceive the new query as an improvement that *hides* a lot of the complexity. That's also one of the reasons why I want to get rid of the MatchQuery. All who argues in favor of the MatchQuery also saw the birth of it and has lived with it ever since so you perceive it as something "simple". I came in much later and think it's an abomination that destroys the clean concept of what a query usually is. I also see that it revokes all control over the actual evaluation from the evaluator. (In reply to comment #11) > It is not about the time it takes to change a few lines of code. It is the > overall delay and the impact this will have for our consumers to release on > time. I understand that. I'm just concerned that in real life, there might be very few such consumers with just a handful of queries and that the light pebble in shoe that this change would cause is very easy to remedy. Where are those consumers? And why are they implementing things on a deprecated interface that is stated to go away in the next milestone? I'll stop arguing now. Everyone knows my point of view :-)
I have given this issue a lot of thought, and I'm going to have to vote -1 for removing match query at this time. I completely agree with Thomas H. on the need for cleanliness and the advantages of the query language, however, without a simple upgrade path (and no, having Thomas convert all existing clients is not simple) I don't think we should make this change. I think providing a post filter would work, but Tom W. is correct, it is an API change, and a non-trivial one at that. If we do decide to leave the API, we should decide what to do about the deprecation warning. I would be happy to leave it if we felt we could honestly remove the class in the future. If we do leave the warning, we should update the text (it currently hints that the class might be removed in 3.6M7). On a final note, I just want to say that I think the p2ql is pretty amazing engineering feat. I didn't think we could completely replace the existing query structure with a custom language and convert all the queries to this new language -- but Thomas proved me wrong. I think the language provides all sorts of interesting possibilities for us.
We seem to have agreed on the explicit decision to keep MatchQuery on the p2-dev mailing list and in this bug report. Now the remaining question is what to do with the deprecation statement. I think the current message is incorrect. We can discuss the need to still have a deprecation statement if you think it is necessary to point folks to the p2 ql as the alternative way but I don't think we should have a message stating that the API may be removed in the javadoc.
The reason I implemented the QL was to get rid of opaque queries since they make some things virtually impossible. Consider the following: Today, the planner requires everything in memory. The traverse() method was carefully crafted to deliver a limited result that the planner can peruse based on a set of roots. The memory problem will remain as long as we use big XML files as the only source of data but other solutions are already percolating. Another reason for getting rid of the opaque queries was that we repositories stored in a database. We see that in a very near future, it will become interesting to aggregate a large amount of IU's into a database. The QL is designed so that it can make good use of database indexes and memory efficiency. A third reason is that p2 could be remoted and make use of things like map/reduce. A network is very easy to build up using composites but today network will rely entirely on "dumb" servers and all nodes must be perused in the client. By distributing queries, data can be collected and assembled per node. With MatchQueries, none of the above use-cases are feasible and the major reasons for having the QL in the first place are gone! Allegedly, consumers out there write queries against a deprecated interface. This is apparently a huge concern although none of them has raised any issues. My hope is that they actually see and understand the benefits of not using a MatchQuery. If we remove the deprecation note, then the MatchQuery is back as a fully sanctioned way of writing queries. To us, that would be really horrible, not to say devastating. Personally, I see it as a major blow that will have a very bad impact on p2's future as a scalable provisioning solution. So can someone please say again, what the reasons are for sacrificing a perfectly good solution that makes the above cases possible for an alternative solution that doesn't enable anything but renders my whole effort more or less useless?
In case it has any effect - I fully agree with Thomas, I am not at all in favour of keeping the (I)MatchQuery. I have not seen any argument that IMO merits that it should be kept, and I am quite frankly upset that all the work put into the p2QL for the very sake of getting rid of constructions that prevents using remote and large repositories, and allow us to have a solid product offering on p2 technology is effectively killed by this decision. I can live with deprecation, but it needs to have a scary message to make users realize that it should not be used at all, and that it indeed will be removed. The sooner the better. The deprecation message states the fact that it will likely go away at a later milestone - anyone writing code against p2 must be aware that p2's API is emerging and should have paid attention.
I'm not sure what the issue is here. 1). it is past the API freeze 2). multiple PMC members have given a -1 3). there are current clients who will be broken if this is removed Given the number of broken builds that our team has been responsible for in the past 6 weeks along with the number of API changes we have done and the community's perception that "p2 is always breaking things" I think we just need to declare this done and move on.
Having talked to many people at eclipsecon, it looks obvious that very large repositories are coming and will probably be there by year end. Consequently, leaving IMatchQuery for sake of "simplicity" is just providing the ideal rope for our clients to hang themselves which I think is not fair. On top of that it is also exposing ourselves to more criticism when ppl will have written bad queries. Sure p2QL is scary, but removing IMatchQuery does not force the user to learn p2QL. This is the fundamental misunderstanding I see here. Instead, people can start by composing the existing queries (QueryUtil) and move the additional computation they use to do in Collector in an iterator on the result of the new query. Also given that this was supposed to be removed in M7, I want to discuss its removal in 3.7 right now and come to an understanding that this class will be gone when 3.7 M1 ship. Failing to find an agreement on that I will have to vote +1 to continue with this work in 3.6.
(In reply to comment #16) > With MatchQueries, none of the above use-cases are feasible and the major > reasons for having the QL in the first place are gone! There are many clients of p2, using it in many different ways. For some of these clients, none of the above concerns matter. They are not using large remote repositories, database-backed repositories, etc. There are others that will want to make use of such repository technologies, and for them p2ql will be very valuable. I don't see how one camp making use of MatchQuery has any impact on the other - they are separate client worlds that will never overlap. I suggest if there ever is a repository that exploits the remoting capabilities of expression queries, that the repository be specified to only accept expression queries, and have it thrown an exception if a client attempts to use a MatchQuery on it. That way you can be sure that the power of p2ql in that environment can be fully exploited, and clients that want to use such repositories must use expression queries to take advantage of it. This way you are only imposing that constraint in environments where it matters. > So can someone please say again, what the reasons are for sacrificing a > perfectly good solution that makes the above cases possible for an alternative > solution that doesn't enable anything but renders my whole effort more or less > useless? Again, I don't see your efforts on p2ql as useless. For clients that will be working in environments that you have in mind (remote/database repos, etc), they will benefit greatly. If there exists other clients that use MatchQuery but never interact with your repositories, it doesn't have any impact on you at all. If remote/database repositories turn out to be really compelling for these clients, then they will make the investment to migrate over to use expression queries on their own. All I'm advocating is that we give clients options and allow them to make their own decisions, rather than trying to force something on them that they may never need.
(In reply to comment #20) > I don't see how one camp making use of MatchQuery has any impact > on the other - they are separate client worlds that will never overlap. > I completely fail to see how you would prevent that overlap, or how you can control what clients that use what type of query once it's been declared a sanctioned and public API. > I suggest if there ever is a repository that exploits the remoting capabilities > of expression queries, that the repository be specified to only accept > expression queries, and have it thrown an exception if a client attempts to use > a MatchQuery on it. How is that better then having a solution that always works, regardless of what type of repository that is used? I can see major negative implications with that approach. > All I'm advocating is that we give clients options and allow them to make > their own decisions, rather than trying to force something on them that > they may never need. You are advocating a solution that brings no benefits at all at the expense of leaving the guts of the query engine wide open. I can live with us keeping the construct in there for now, as long the deprecation and the note about future removal stays.
I agree with comment 20. (In reply to comment #19) > Also given that this was supposed to be removed in M7, I want to discuss its > removal in 3.7 right now and come to an understanding that this class will be > gone when 3.7 M1 ship. > Failing to find an agreement on that I will have to vote +1 to continue with > this work in 3.6. There were several separate discussions the week before M6 about MatchQuery. At one meeting everyone agreed to keep MatchQuery because of the issues John outlined in comment 20. Another meeting (literally the day before M6), John and DJ were not present and there was much talk of removing MatchQuery again. There was no way we could remove that close to M6 let along ignore the issues John and DJ were concerned about without letting them voice their concerns. We came to a compromise to add a deprecation statement to the MatchQuery so we could at least make it clear in the javadoc of MatchQuery that p2 ql is the preferred way. Once the dust settled on M6 we have revisited the decision to remove MatchQuery. It is clear that removing the MatchQuery will be very disruptive to several p2 clients. We have closed the issue on the mailing list and have agreed that the API will stay for Helios. Now we are talking about a preemptive decision to break compatibility for the next release. And we have to make the decision to break compatibility now or else remove MatchQuery in 3.6. We cannot remove MatchQuery in 3.6. The only way I can see agreement on removing MatchQuery in the future is if there is a compatibility story where MatchQuery can be available with some compatibility fragment.
Given the recent discussions about the QL, and the fact that some of the people involved stems from OTI, I decided to rewrite the parser to lower the threshold for acceptance. The new syntax will be Smalltalk. A patch is imminent and I hope that everyone agrees that it's worth while to get this change in prior to M7 even it it means that some clients must rewrite some of their code. I'll of course help with that.
Created attachment 163640 [details] Smalltalk parser, based on OMG standard Please review.
(In reply to comment #21) > I completely fail to see how you would prevent that overlap, or how you can > control what clients that use what type of query once it's been declared a > sanctioned and public API. You can never control what clients do. Without MatchQuery they could do equally damaging things using QueryUtil#createIUAnyQuery(). No API can enforce "good" client behaviour. All you can do is provide the expression query API, and clients will use if if they see a compelling advantage to doing so. > How is that better then having a solution that always works, regardless of > what type of repository that is used? I can see major negative implications > with that approach. It's better because it is explicit. If we remove MatchQuery, the client code will always work, but it will be essentially unusable on large remote repositories if they are using IUAnyQuery, MatchIterator, or any other similar approach. If your repository could recognize those unoptimized queries it could provide a much more informative response, and the client would immediately know what they need to do in order to work with your repositories. > You are advocating a solution that brings no benefits at all at the expense of > leaving the guts of the query engine wide open. I can live with us keeping the > construct in there for now, as long the deprecation and the note about future > removal stays. There are clear benefits to MatchQuery that I have articulated several times. The short summary is that MatchQuery implementations are written in Java. This means they will often be faster where indexes aren't used, because a compiled language is generally faster than an interpreted one. For example PerformanceTest#testMatchQueryVersusExpressionPerformance where the MatchQuery is still much faster than the expression query (400ms vs 640ms for me). More importantly, it means clients can understand them, they can step through them in the debugger and see them working. They don't need to learn a new language just to get some information out of a profile. If you don't even acknowledge that such benefits exist I don't think I can continue having a rational discussion about this.
(In reply to comment #25) > You can never control what clients do. Without MatchQuery they could do equally > damaging things using QueryUtil#createIUAnyQuery(). > Yes, but that's a very conscious decision where it's clear beyond all doubt that you are indeed selecting everything. It's like issuing a query against a database without a WHERE clause. > No API can enforce "good" client behaviour. No, but it can certainly encourage it. > All you can do is provide the expression query API, and > clients will use if if they see a compelling advantage to doing so. > That's not all I can do. I can also remove damaging constructs to the extent possible. I.e., you don't refrain from putting a lid over a well just because someone might remove it and jump in. > It's better because it is explicit. If we remove MatchQuery, the client code > will always work, but it will be essentially unusable on large remote > repositories if they are using IUAnyQuery, MatchIterator, or any other similar > approach. Not sure I understand that assumption. Why would it be unusable? It might be slow in some cases, but at least it will work. One less "p2 break things" to worry about. > If your repository could recognize those unoptimized queries it could > provide a much more informative response, and the client would immediately know > what they need to do in order to work with your repositories. > I can think of several optimization's, guards, etc. that repositories might implement. With the expression queries, everything is possible, including informative responses when blocking certain kinds of queries. It's also relatively easy since there is one uniform way of examining the query and make decisions. > There are clear benefits to MatchQuery that I have articulated several times. > The short summary is that MatchQuery implementations are written in Java. This > means they will often be faster where indexes aren't used, because a compiled > language is generally faster than an interpreted one. > That assumes that query authors always write very efficient code. History shows that this is often not the case. A lot (perhaps even a majority) of the queries that was rewritten in p2 perform faster now. > For example PerformanceTest#testMatchQueryVersusExpressionPerformance > where the MatchQuery is still much faster than the expression query (400ms vs > 640ms for me). Yes, that test is explicitly designed to show a situation where the MatchQuery can perform better. It uses a string.startsWith() and the expression uses a match. Here are the results from my machine, including all tests involving MatchQuery. Take a look at the IteratorFilter in the last group. That's the alternative we propose in the deprecation note: ExpressionQuery took: 1524 milliseconds MatchQuery took: 1091 milliseconds IndexedExpressionQuery took: 27 milliseconds MatchQuery took: 546 milliseconds IndexedExpressionQuery took: 76 milliseconds MatchQuery took: 2866 milliseconds MatchFilter took: 523 milliseconds IteratorFilter took: 433 milliseconds MatchQuery took: 1131 milliseconds (for the uninitiated, the times reflect queries that execute up to 1000 times on a repository with about 4000 items) > More importantly, it means clients can understand them, they can step through them > in the debugger and see them working. They don't need to learn a new language > just to get some information out of a profile. If you don't even acknowledge > that such benefits exist I don't think I can continue having a rational > discussion about this. Of course those benefits exists, but they are not treats of the MatchQuery. The alternative solution proposed in the deprecation note will give you this too, and in a perhaps even more understandable way. It boils down to where the java code resides. I don't want the java code passed down to the query engine since that a) breaks all scenarios where it doesn't execute this code in-process, b) can give strange results caused by side-effects when the java code alters things, and c) might cause deadlocks etc. when using a multi-threaded evaluator.
afaikt, there simply are NO technical merits in keeping (I)MatchQuery, only downside. The technical arguments to keep MatchQuery, irrespective of how many time they are presented, have been refuted just as many times. So - look at the facts: - it is not faster to use MatchQuery - it is not more difficult to use the alternative - it is just as easy to debug the alternative Agree to the deprecation to give people time to adjust - but the construct should be removed asap. And, as Thomas said, we will rewrite the queries for anyone that is actually using MatchQuery.
Folks, this is not productive. There comes a time in a release cycle where you just don't do certain changes. We have passed that time for this change. The whole point of an API freeze is that consuming teams do not have to deal with change after the freeze. It is painful and there are many times where each of us has suffered for not having gotten our changes in before the freeze. Alas, barring critical bugs or obviously fringe cases, the freeze is a freeze. You might be tempted to argue for the latter case. Please resist. There are people on this bug standing up and saying that the function is used and not fringe. Please believe them. Your kind offer to help people adopt is great. However, work is often not the problem. *Change* is the problem. Teams shipping on Eclipse are focused on their code and after the freeze, can and should not be dealing with change in the lower layers. I believe we all have lots of stuff to do so lets get on with it.
I have updated the deprecation comment to remove the line about the API being removed in M7.
(In reply to comment #28) > Folks, this is not productive. There comes a time in a release cycle where you > just don't do certain changes. We have passed that time for this change. The > whole point of an API freeze is that consuming teams do not have to deal with > change after the freeze. Exactly. And we have been very active in making this change take place. I am quite frankly upset that Inertia pays off - delaying the decision until it is too late, and then hiding behind that even if there are close to no clients outside the group closely related to the p2 team. Exactly which teams are affected by this change again? Exactly how many voiced concerns in this now long running debate? Who could have missed the prominent deprecation warning if they were using the (I)MatchQuery? People working on p2 are certainly aware that the API is emerging and must have payed close attention to what has been going on with the API. BTW, on this particular day ;-) it may be in place to question if the removal of the deprecation warning about M7 removal is an API change in itself as we certainly rely on the fact that IMatchQuery is to be removed. Can we at least have a deprecation comment that states that it will be removed in Eclipse 3.7 (as Pascal also suggest) so people understand that this is not something that will be maintained for ever in deprecated state. Please, let us not repeat this game for 3.7.
(In reply to comment #28) > Folks, this is not productive. There comes a time in a release cycle where you > just don't do certain changes. We have passed that time for this change. The > whole point of an API freeze is that consuming teams do not have to deal with > change after the freeze. I would also very much like to put this aside and work on other things. In the aftermath of the discussion there are two things I'd like to have clarified: 1. Please correct me if I'm wrong, but from what all that I know, the MatchQuery never made API as anything but provisional (pre M6) or deprecated with a notice about imminent removal. It has *never* been an officially sanctioned API. My point is that arguing that the MatchQuery must stay because we're following strict rules about API freeze is in fact very weak. If clients start using it now, at M6, they do that against all API recommendations. What I'd like to have clarified, is why it's a big concern if we break those clients, especially if we do all in our power to help them get passed the hurdle? 2. Who are the known clients affected by this? Can someone please help me to get in contact with them. The reason for this is that I want to inform as many as possible about the cons in using the MatchQuery and offer my help with alternatives.
