-
Notifications
You must be signed in to change notification settings - Fork 610
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
Support YDB in YQL Generic Provider (YQv1) #2300
Changes from all commits
16aa7a9
8418d3d
be5cfd7
193e834
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
#include "database_resolver.h" | ||
|
||
#include <util/string/split.h> | ||
#include <ydb/core/fq/libs/common/cache.h> | ||
#include <ydb/core/fq/libs/config/protos/issue_id.pb.h> | ||
#include <ydb/core/fq/libs/events/events.h> | ||
|
@@ -136,7 +137,7 @@ class TResponseProcessor : public TActorBootstrapped<TResponseProcessor> | |
const auto requestIter = Requests.find(ev->Get()->Request); | ||
HandledIds++; | ||
|
||
LOG_T("ResponseProcessor::Handle(HttpIncomingResponse): got MDB API response: code=" << ev->Get()->Response->Status); | ||
LOG_T("ResponseProcessor::Handle(HttpIncomingResponse): got API response: code=" << ev->Get()->Response->Status); | ||
|
||
try { | ||
HandleResponse(ev, requestIter, errorMessage, result); | ||
|
@@ -312,7 +313,12 @@ class TDatabaseResolver: public TActor<TDatabaseResolver> | |
} | ||
|
||
Y_ENSURE(endpoint); | ||
return TDatabaseDescription{endpoint, "", 0, database, secure}; | ||
|
||
TVector<TString> split = StringSplitter(endpoint).Split(':'); | ||
|
||
Y_ENSURE(split.size() == 2); | ||
|
||
return TDatabaseDescription{endpoint, split[0], FromString(split[1]), database, secure}; | ||
}; | ||
Parsers[NYql::EDatabaseType::Ydb] = ydbParser; | ||
Parsers[NYql::EDatabaseType::DataStreams] = [ydbParser]( | ||
|
@@ -327,9 +333,11 @@ class TDatabaseResolver: public TActor<TDatabaseResolver> | |
if (!isDedicatedDb && ret.Endpoint.StartsWith("ydb.")) { | ||
// Replace "ydb." -> "yds." | ||
ret.Endpoint[2] = 's'; | ||
ret.Host[2] = 's'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. избыточность. нет смысл заменить endpoint на два поля host + port ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Такие мысли были, но думаю, что точно не в этом ревью, так как это изменение затронет много провайдеров в yql. |
||
} | ||
if (isDedicatedDb) { | ||
ret.Endpoint = "u-" + ret.Endpoint; | ||
ret.Host = "u-" + ret.Host; | ||
} | ||
return ret; | ||
}; | ||
|
@@ -486,6 +494,7 @@ class TDatabaseResolver: public TActor<TDatabaseResolver> | |
try { | ||
TString url; | ||
if (IsIn({NYql::EDatabaseType::Ydb, NYql::EDatabaseType::DataStreams }, databaseType)) { | ||
YQL_ENSURE(ev->Get()->YdbMvpEndpoint.Size() > 0, "empty YDB MVP Endpoint"); | ||
url = TUrlBuilder(ev->Get()->YdbMvpEndpoint + "/database") | ||
.AddUrlParam("databaseId", databaseId) | ||
.Build(); | ||
|
@@ -497,7 +506,6 @@ class TDatabaseResolver: public TActor<TDatabaseResolver> | |
.AddPathComponent("hosts") | ||
.Build(); | ||
} | ||
LOG_D("ResponseProccessor::Handle(EndpointRequest): start GET request: " << url); | ||
|
||
NHttp::THttpOutgoingRequestPtr httpRequest = NHttp::THttpOutgoingRequest::CreateRequestGet(url); | ||
|
||
|
@@ -507,6 +515,8 @@ class TDatabaseResolver: public TActor<TDatabaseResolver> | |
httpRequest->Set("Authorization", token); | ||
} | ||
|
||
LOG_D("ResponseProccessor::Handle(EndpointRequest): start GET request: " << "url: " << httpRequest->URL); | ||
|
||
requests[httpRequest] = TResolveParams{databaseId, databaseType, databaseAuth}; | ||
} catch (const std::exception& e) { | ||
const TString msg = TStringBuilder() << "error while preparing to resolve database id: " << databaseId | ||
|
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.
Bearer - важная тема. хорошо бы ее тоже запилить. это нужно для работы внутри Я не с помощью OAuth токена, а с помощью IAM токена
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.
Ты имеешь в виду, добавить опцию
AddBearerToToken
вот сюда https://github.com/ydb-platform/ydb/blob/main/ydb/library/yql/providers/common/proto/gateways_config.proto#L547-L590, чтобы индивидуально указывать каждому кластеру?Сейчас при ресолвинге безусловно true. Это неправильно? https://github.com/ydb-platform/ydb/blob/main/ydb/library/yql/providers/generic/provider/yql_generic_settings.cpp#L52
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.
пусть будет true, нормуль
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.
Ещё раз хочу по пунктам проговорить, что надо делать с bearer, потому что логика весьма запутанная и есть разные контексты применения bearer.
Поход в MDB API, в YDB CP (за ресолвингом)
Bearer
всегда true. Так всё и остаётся.Поход в YDB CP (за метаданными/данными)
Сейчас
Bearer
всегда false. Надо:TGenericClusterConfig
полеAddBearerToToken
.clusters_from_connections.cpp
, брать его из конфига YQ.TGenericProvider::AddCluster
. Это означает, чтоAddBearerToToken
просочится в DSL (как минимум в параметрыCREATE EXTERNAL DATA SOURCE
для YDB).Всё верно?
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.
вроде верно. это доп флаг для аутентификации во внешней системе