ISSUES-5436 support custom http by zhang2014 · Pull Request #7572 · ClickHouse/ClickHouse

zhang2014 · 2019-11-01T11:50:48Z

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):

New Feature

Short description (up to few sentences):
#5436

zhang2014 · 2019-11-11T14:38:03Z

@ValBaturin can you help me review this pr?

cc: @alexey-milovidov @millb

ValBaturin · 2019-11-12T13:04:41Z

@zhang2014 sure

ValBaturin

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.

ValBaturin · 2019-11-12T18:50:06Z

dbms/src/Common/HTMLForm.h

Is it used anywhere?

https://github.com/ClickHouse/ClickHouse/pull/7572/files#diff-eacd15bdb61e3f8be33f372169a6276eR97

ValBaturin · 2019-11-12T18:58:42Z

dbms/src/Interpreters/Context.cpp

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.

ValBaturin · 2019-11-12T19:39:53Z

dbms/tests/queries/0_stateless/00408_http_keep_alive.sh

Why is it removed?

For 404 test will be move to integration test. for no keep-alive we are currently not compatible.

ValBaturin · 2019-11-12T19:45:18Z

dbms/src/Interpreters/ReplaceQueryParameterVisitor.cpp

Is this file changed unintentionally?

ValBaturin

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.

ValBaturin · 2019-11-14T20:26:58Z

dbms/src/Interpreters/CustomHTTP/CustomExecutor.h

It must be a typo, right?

ValBaturin · 2019-11-14T20:48:13Z

dbms/programs/server/HTTPHandler.cpp

Extractor Client Info is only used in here. I'd suggest to make it local since it shouldn't be in interpreter anyway.

zhang2014 · 2019-11-15T14:02:35Z

@ValBaturin Thanks your review

Executor family doesn't fit into the current interpreter design. HTTPInputStreams and HTTPOutputStreams should be defined in DataStreams or at least in server.

I move them into DataStreams

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.

~~I'll rename them. what do you recommend?~~

As mentioned above, I rename them to predefined queries.

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.

~~Because we put the CustomExecutor into context, if we put the CustomExecutor into the programs the will appear interpreter dependent programs~~

Now they are refactored into the programs

zhang2014 · 2019-11-22T10:44:21Z

@ValBaturin Can you help me review this PR again?

zhang2014 · 2020-04-27T10:44:16Z

@alexey-milovidov @filimonov

Sorry for delay. I think this pr is ready for review

qoega · 2020-04-27T12:06:32Z

Can you remove also here

ClickHouse/programs/server/ya.make

Line 21 in 17e7d4d

PingRequestHandler.cpp

alexey-milovidov · 2020-04-27T12:10:08Z

Current reviewer is @tavplubix

Merging #7572

azat · 2020-06-11T13:29:40Z

programs/server/HTTPHandlerFactory.cpp

+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");


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)

@zhang2014 @tavplubix thoughts?

filimonov · 2020-06-11T13:56:50Z

@azat - but standard paths are still there:
https://github.com/ClickHouse/ClickHouse/pull/10547/files#diff-5981515e3ab857757e2ccc763c560d2cR121-R128

ClickHouse/programs/server/HTTPHandlerFactory.cpp

Lines 121 to 135 in aeac8cb

    
           auto factory = (new HTTPRequestHandlerFactoryMain(name)) 
        
               ->addHandler((new HandlingRuleHTTPHandlerFactory<StaticRequestHandler>(server, root_response_expression)) 
        
                   ->attachStrictPath("/")->allowGetAndHeadRequest()) 
        
               ->addHandler((new HandlingRuleHTTPHandlerFactory<StaticRequestHandler>(server, ping_response_expression)) 
        
                   ->attachStrictPath("/ping")->allowGetAndHeadRequest()) 
        
               ->addHandler((new HandlingRuleHTTPHandlerFactory<ReplicasStatusHandler>(server)) 
        
                   ->attachNonStrictPath("/replicas_status")->allowGetAndHeadRequest()) 
        
               ->addHandler((new HandlingRuleHTTPHandlerFactory<DynamicQueryHandler>(server, "query"))->allowPostAndGetParamsRequest()); 
        
           if (server.config().has("prometheus") && server.config().getInt("prometheus.port", 0) == 0) 
        
               factory->addHandler((new HandlingRuleHTTPHandlerFactory<PrometheusRequestHandler>( 
        
                   server, PrometheusMetricsWriter(server.config(), "prometheus", async_metrics))) 
        
                       ->attachStrictPath(server.config().getString("prometheus.endpoint", "/metrics"))->allowGetAndHeadRequest()); 
        
           return factory;

or you mean that it can be overwritten by a custom config?

azat · 2020-06-11T14:07:59Z

@azat - but standard paths are still there:

I see, but it is under else, so with enabled http_handlers they do not work

filimonov · 2020-06-11T14:15:03Z

@azat - but standard paths are still there:

I see, but it is under else, so with enabled http_handlers they do not work

Indeed. That's crap.

We need to push standard handers (ping / replica_status / prometheus) before user-defined handlers always.

alexey-milovidov · 2020-06-11T14:31:39Z

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?

tavplubix · 2020-06-11T14:42:00Z

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.

filimonov · 2020-06-11T14:42:10Z

I though default configuration lists a way how to configure /ping & /replica_status to return them back, but it's not. For /replicas_status it seems not possible, same for /prometeus

tavplubix · 2020-06-11T14:59:55Z

I will add more configuration examples and ability to configure ReplicasStatusHandler and PrometheusRequestHandler.

filimonov · 2020-06-11T15:14:09Z

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.
For example, if the user will decide to remove /ping handler he will just configure.

        <rule>
            <url>/ping</url>
            <handler>
                <type>static</type>
                 <status>404</status>
            </handler>
        </rule>

and it will have higher priority.

filimonov · 2020-06-11T15:16:35Z

And i think the more common use case is to extend standatd handlers, not replace all of them.

alexey-milovidov · 2020-06-11T17:07:11Z

@filimonov There is a use case - to build limited API without access to default handlers.

We can:

provide example directly in config.xml on how to configure default handlers;
add support for <default/> specification on the same level as <rule> to automatically enable all default handlers.

zhang2014 force-pushed the feature/ISSUES-5436 branch 11 times, most recently from cfdaffd to 8e1d6ed Compare November 8, 2019 11:39

zhang2014 marked this pull request as ready for review November 11, 2019 14:38

zhang2014 requested a review from a team November 11, 2019 14:38

ghost requested review from millb and removed request for a team November 11, 2019 14:38

zhang2014 force-pushed the feature/ISSUES-5436 branch 2 times, most recently from 810e29a to 96d6cdf Compare November 12, 2019 05:22

zhang2014 force-pushed the feature/ISSUES-5436 branch from 96d6cdf to ce2cf0e Compare November 13, 2019 04:12

ValBaturin reviewed Nov 13, 2019

View reviewed changes

ValBaturin reviewed Nov 14, 2019

View reviewed changes

zhang2014 force-pushed the feature/ISSUES-5436 branch 4 times, most recently from 4931649 to 84e6c08 Compare November 22, 2019 10:43

zhang2014 force-pushed the feature/ISSUES-5436 branch 2 times, most recently from 2d26e4e to 749401b Compare November 22, 2019 12:23

ISSUES-5436 reworking predefine http

57cbecf

zhang2014 force-pushed the feature/ISSUES-5436 branch from e1716d3 to 57cbecf Compare April 19, 2020 19:23

zhang2014 added 2 commits April 20, 2020 12:49

ISSUES-5436 try fix build failure & pvs & style

318ab3b

ISSUES-5436 fix integration test failure & add test

0070f75

zhang2014 force-pushed the feature/ISSUES-5436 branch from 46b5de5 to 0070f75 Compare April 23, 2020 09:11

zhang2014 changed the title ~~[WIP] ISSUES-5436 support custom http~~ ISSUES-5436 support custom http Apr 27, 2020

zhang2014 force-pushed the feature/ISSUES-5436 branch from b27fb20 to 4739245 Compare April 27, 2020 10:41

ISSUES-5436 add integration test

8123094

zhang2014 force-pushed the feature/ISSUES-5436 branch from 4739245 to 8123094 Compare April 27, 2020 10:49

ISSUES-5436 fix ya make

d56002b

tavplubix mentioned this pull request Apr 27, 2020

Merging #7572 #10547

Merged

tavplubix added a commit that referenced this pull request Apr 28, 2020

Merge pull request #10547 from ClickHouse/zhang2014/feature/ISSUES-5436

11c3493

Merging #7572

tavplubix merged commit d56002b into ClickHouse:master Apr 28, 2020

azat reviewed Jun 11, 2020

View reviewed changes

zhang2014 mentioned this pull request Jun 12, 2020

ISSUES-7572 support config default http handler #11628

Merged

BayoNet mentioned this pull request Jun 17, 2020

DOCS-630: The Predefined HTTP Handlers topic translated to Russian #11738

Merged

azat mentioned this pull request Jul 27, 2020

Additional metrics added to Prometheus endpoint #12647

Closed

Conversation

zhang2014 commented Nov 1, 2019

Uh oh!

zhang2014 commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValBaturin commented Nov 12, 2019

Uh oh!

ValBaturin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValBaturin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhang2014 commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhang2014 commented Nov 22, 2019

Uh oh!

zhang2014 commented Apr 27, 2020

Uh oh!

qoega commented Apr 27, 2020

Uh oh!

alexey-milovidov commented Apr 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filimonov commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azat commented Jun 11, 2020

Uh oh!

filimonov commented Jun 11, 2020

Uh oh!

alexey-milovidov commented Jun 11, 2020

Uh oh!

tavplubix commented Jun 11, 2020

Uh oh!

filimonov commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tavplubix commented Jun 11, 2020

Uh oh!

filimonov commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filimonov commented Jun 11, 2020

Uh oh!

alexey-milovidov commented Jun 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

zhang2014 commented Nov 11, 2019 •

edited

Loading

zhang2014 commented Nov 15, 2019 •

edited

Loading

filimonov commented Jun 11, 2020 •

edited

Loading

filimonov commented Jun 11, 2020 •

edited

Loading

filimonov commented Jun 11, 2020 •

edited

Loading