search

lyft / presto-gateway

A load balancer / proxy / gateway for prestodb
Apache License 2.0
358 stars 156 forks source link

Cannot not extract queryId exactly in code, is that a bug? #135

Open BruceKellan opened 3 years ago

BruceKellan commented 3 years ago

Hi, thanks for your repo, it give me a lot of inspiration. But I found a bug, this is code segment: https://github.com/lyft/presto-gateway/blob/39d7d5f3194265c688d4d5f01db1d64143fe284a/gateway-ha/src/main/java/com/lyft/data/gateway/ha/handler/QueryIdCachingProxyHandler.java#L156

It use a overload method extractQueryIdIfPresent(path, queryParams), and the test case TestQueryIdCachingProxyHandler test extractQueryIdIfPresent(path, queryParams).

But if the request url is "/ui/query.html?20200416_160256_03078_6b4yt", path will be "/ui/query.html" and queryParams will be "20200416_160256_03078_6b4yt", in this case, we cannot not extract queryId exactly and get null

Logic is not the same as test case. https://github.com/lyft/presto-gateway/blob/39d7d5f3194265c688d4d5f01db1d64143fe284a/gateway-ha/src/test/java/com/lyft/data/gateway/ha/handler/TestQueryIdCachingProxyHandler.java#L26

I think it's a bug, isn't it?

amitds1997 commented 3 years ago

Sorry, if I misunderstood your question, why can we not extract the queryId?

/ui/query.html?20200416_160256_03078_6b4yt satisfies the query Id pattern and should be extractable.

https://github.com/lyft/presto-gateway/blob/39d7d5f3194265c688d4d5f01db1d64143fe284a/gateway-ha/src/main/java/com/lyft/data/gateway/ha/handler/QueryIdCachingProxyHandler.java#L42

This is used when a Trino UI path is passed to the gateway.

https://github.com/lyft/presto-gateway/blob/39d7d5f3194265c688d4d5f01db1d64143fe284a/gateway-ha/src/main/java/com/lyft/data/gateway/ha/handler/QueryIdCachingProxyHandler.java#L204

BruceKellan commented 3 years ago

Hi, amitds1997, thanks for your reply. My question is that the variable path is request.getRequestURI() https://github.com/lyft/presto-gateway/blob/39d7d5f3194265c688d4d5f01db1d64143fe284a/gateway-ha/src/main/java/com/lyft/data/gateway/ha/handler/QueryIdCachingProxyHandler.java#L157 So the variable path will be /ui/query.html without 20200416_160256_03078_6b4yt, the variable queryParams will be 20200416_160256_03078_6b4yt in this case.

So regexp cannot extract queryId because path is /ui/query.html.

The above is my understanding. I reproduce it in my code.

If there is a problem with my understanding, please tell me.

By the way, my prestosql version is 348, /ui/query.html?20200416_160256_03078_6b4yt is my PrestoSQL UI path。

BruceKellan commented 3 years ago

I have looked at other fork repo, such as rongfengliang's presto-gateway, In his repo, he use the queryParams to be queryId in the same place.

https://github.com/rongfengliang/presto-gateway/blob/7be1c438868b059b375e3af91e40ab2a061988ca/gateway/src/main/java/com/lyft/data/gateway/handler/QueryIdCachingProxyHandler.java#L201

You can have a see.

amitds1997 commented 3 years ago

Yes, that seems to be a bug.