New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ISSUES-5436 support custom http #7572
Conversation
cfdaffd
to
8e1d6ed
Compare
@ValBaturin can you help me review this pr? |
810e29a
to
96d6cdf
Compare
@zhang2014 sure |
96d6cdf
to
ce2cf0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like most of the logic is implemented in CustomHTTP
interpreter and http handler from server
partially moved to the interpreter. I haven't fully read it yet though.
In my opinion, customHTTP name is misleading, and I'd suggest something like predefined queries.
I'll finish the review by tomorrow.
dbms/src/Common/HTMLForm.h
Outdated
@@ -26,6 +26,12 @@ struct HTMLForm : public Poco::Net::HTMLForm | |||
readUrl(istr); | |||
} | |||
|
|||
template <typename T> | |||
bool check(const std::string & key, T check_value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dbms/src/Interpreters/Context.cpp
Outdated
@@ -186,6 +189,9 @@ struct ContextShared | |||
std::unique_ptr<Clusters> clusters; | |||
ConfigurationPtr clusters_config; /// Soteres updated configs | |||
mutable std::mutex clusters_mutex; /// Guards clusters and clusters_config | |||
std::unique_ptr<CustomExecutors> custom_executors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to separate this section from clusters(187 - 191) by a newline or a comment line or just move it somewhere else. Otherwise it looks like it's all about clusters for distributed tables.
@@ -8,6 +8,3 @@ URL="${CLICKHOUSE_PORT_HTTP_PROTO}://${CLICKHOUSE_HOST}:${CLICKHOUSE_PORT_HTTP}/ | |||
${CLICKHOUSE_CURL} -vsS ${URL} --data-binary @- <<< "SELECT 1" 2>&1 | perl -lnE 'print if /Keep-Alive/'; | |||
${CLICKHOUSE_CURL} -vsS ${URL} --data-binary @- <<< " error here " 2>&1 | perl -lnE 'print if /Keep-Alive/'; | |||
${CLICKHOUSE_CURL} -vsS ${URL}ping 2>&1 | perl -lnE 'print if /Keep-Alive/'; | |||
|
|||
# no keep-alive: | |||
${CLICKHOUSE_CURL} -vsS ${URL}404/not/found/ 2>&1 | perl -lnE 'print if /Keep-Alive/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 404 test will be move to integration test
. for no keep-alive
we are currently not compatible.
@@ -6,6 +6,7 @@ | |||
#include <DataTypes/IDataType.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file changed unintentionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Executor family doesn't fit into the current interpreter design. HTTPInputStreams
and HTTPOutputStreams
should be defined in DataStreams
or at least in server
. CustomHTTP
name as it was already mentioned above is misleading e.g. it might mean an interpreter of modification of http protocol but an interpreter of custom HTTP handlers with prepared queries. I also don't really get why CustomExecutor
is named that way.
To be honest I'm not sure if it's a job of an interpreter at all. Yeah, I see that you do some AST modifications and it's indeed an interpretation stage but overall dealing with HTTPServerRequest
and HTTPServerResponse
feels more like a server job.
I'd be happy to hear your elaboration on such a design decision and any thoughts of other people who've been involved in the project for a while or have a better grasp on the subject.
class CustomExecutor | ||
{ | ||
public: | ||
bool canBeParseRequestBody() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It must be a typo, right?
dbms/programs/server/HTTPHandler.cpp
Outdated
@@ -35,6 +35,10 @@ | |||
#include <DataStreams/IBlockInputStream.h> | |||
#include <Interpreters/executeQuery.h> | |||
#include <Interpreters/Quota.h> | |||
#include <Interpreters/CustomHTTP/CustomExecutor.h> | |||
#include <Interpreters/CustomHTTP/HTTPInputStreams.h> | |||
#include <Interpreters/CustomHTTP/ExtractorClientInfo.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extractor Client Info is only used in here. I'd suggest to make it local since it shouldn't be in interpreter anyway.
@ValBaturin Thanks your review
I move them into DataStreams
As mentioned above, I rename them to predefined queries.
Now they are refactored into the |
4931649
to
84e6c08
Compare
@ValBaturin Can you help me review this PR again? |
2d26e4e
to
749401b
Compare
e1716d3
to
57cbecf
Compare
46b5de5
to
0070f75
Compare
b27fb20
to
4739245
Compare
Sorry for delay. I think this pr is ready for review |
4739245
to
8123094
Compare
Can you remove also here ClickHouse/programs/server/ya.make Line 21 in 17e7d4d
|
Current reviewer is @tavplubix |
static inline Poco::Net::HTTPRequestHandlerFactory * createHTTPHandlerFactory(IServer & server, const std::string & name, AsynchronousMetrics & async_metrics) | ||
{ | ||
if (server.config().has("routing_rules")) | ||
return createHandlersFactoryFromConfig(server, name, "routing_rules"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the intention that http_handlers
(it was renamed in merging PR - #10547) overrides default paths? i.e. /ping, /replicas_status
Maybe it will better to use separate server for custom http handlers (on another port)? Or register custom http handlers first and then defaults, to disallow their overrides (or allow overrides, but register anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhang2014 @tavplubix thoughts?
@azat - but standard paths are still there: ClickHouse/programs/server/HTTPHandlerFactory.cpp Lines 121 to 135 in aeac8cb
or you mean that it can be overwritten by a custom config? |
I see, but it is under |
Indeed. That's crap. We need to push standard handers (ping / replica_status / prometheus) before user-defined handlers always. |
If I remember correctly, custom HTTP handlers can be also used to disable or replace default HTTP handlers. It makes sense because it allows to provide limited access to the server. And to keep default HTTP handlers along with custom, you have to add them to configuration. @tavplubix am I right? |
Yes, user may want to configure default handlers other way and it can be done with custom handlers, so current behaviour seems ok to me. |
I though default configuration lists a way how to configure |
I will add more configuration examples and ability to configure |
OK. Actually there is one more option - add standard handers to the handers from config always at the end. So user will be able to overwrite them by his own handlers which will have higher priority.
and it will have higher priority. |
And i think the more common use case is to extend standatd handlers, not replace all of them. |
@filimonov There is a use case - to build limited API without access to default handlers. We can:
|
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
For changelog. Remove if this is non-significant change.
Category (leave one):
Short description (up to few sentences):
#5436