Skip to content
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

CLICKHOUSE-3878: ODBC-Bridge tool implementation #2828

Merged
merged 23 commits into from
Aug 14, 2018

Conversation

alesapin
Copy link
Member

@alesapin alesapin commented Aug 8, 2018

WIP
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

size_t parseMaxBlockSize(const std::string & max_block_size_str, Poco::Logger * log)
{
size_t max_block_size = DEFAULT_BLOCK_SIZE;
if (!max_block_size_str.empty())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should throw an exception if the user specified &max_block_size=

{
try
{
max_block_size = std::stoul(max_block_size_str);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work as expected:

milovidov@milovidov-Pro-P30:~/work/ClickHouse$ g++ -std=c++17 -xc++ -include string - <<< 'int main() { return std::stoul("123abc"); }'
milovidov@milovidov-Pro-P30:~/work/ClickHouse$ ./a.out 
milovidov@milovidov-Pro-P30:~/work/ClickHouse$ echo $?
123

Better to use parse function from ReadHelpers.h

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check for all possibly wrong usages of std::sto... functions in our codebase.

}
catch (...)
{
tryLogCurrentException(log);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to catch exception.

return sample_block;
}

size_t parseMaxBlockSize(const std::string & max_block_size_str, Poco::Logger * log)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this function at all.

return std::make_shared<Poco::Data::SessionPool>("ODBC", connection_str);
}));
}
return pool_map->at(connection_str);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we don't lock mutex here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


ODBCHandler::PoolPtr ODBCHandler::getPool(const std::string & connection_str)
{
if (!pool_map->count(connection_str))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

}
catch (const Exception & ex)
{
process_error("ODBCBridge: Invalid 'columns' parameter in request body '" + ex.message() + "'");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception stack trace is lost.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


std::string format = params.get("format", "RowBinary");
std::string query = params.get("query");
LOG_TRACE(log, "ODBCBridge: Query '" << query << "'");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You put query in single quotes, but query is unescaped - that may lead to confusion, because query often contains single quotes inside.
Better to print query after colon.

{
options.addOption(Poco::Util::Option("http-port", "", "port to listen").argument("http-port", true).binding("http-port"));
options.addOption(
Poco::Util::Option("http-host", "", "hostname to listen, default localhost").argument("http-host").binding("http-host"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All parameters in configuration file that exists in clickhouse-server, should be named exactly as in clickhouse-server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but may I save --log-level in place of --logger.level? I think my variant is much more readable and common. Also, this tool (in contrast to clickhouse-server) provides --help option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Could you please also add --log-level to clickhouse-server?
(--help is also missing, we have a few complaints about it)

while (!checkODBCBridgeIsRunning() && counter <= 5)
{
sleep(1);
counter++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefix increment

while (!checkODBCBridgeIsRunning() && counter <= 5)
{
sleep(1);
counter++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add logging.

{
const auto & config = context_global.getConfigRef();
const auto & settings = context_global.getSettingsRef();
Poco::Path path{config.getString("application.dir", "")};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is application.dir?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/ClickHouse-Extras/poco/blob/3df947389e6d9654919002797bdd86ed190b3963/Util/include/Poco/Util/Application.h#L76 -- It stores information about executable path. I don't know other reliable and cross-platform (linux and macOS) ways to get it from source code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@alesapin
Copy link
Member Author

alesapin commented Aug 10, 2018

I don't understand why travis fails. Seems that the main reason is internal timeouts.

@alesapin
Copy link
Member Author

alesapin commented Aug 14, 2018

Single failed test in Autotests and Travis is

00687_top_and_offset:                                                  
Code: 406. DB::Exception: Can not use TOP and LIMIT together

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants