Skip to content

Conversation

@isapego
Copy link
Contributor

@isapego isapego commented Sep 15, 2022

Done:

  • Implemented basic C++ client. Current functionality: Start client async, get table by name;
  • C++17 was chosen as a target C++ standard. May be changed to C++14 in future, need to discuss it with other C++ guys;
  • Using Conan as a package manager;
  • Using GTest for tests;
  • Compiles on both Windows and Linux;
  • Added simple DEVNOTES.md with instructions;
  • Code style is not yet established and does not match that of a schema lib. To be done in another ticket once we decide on a common code style.

@isapego isapego requested a review from ptupitsyn September 15, 2022 15:42
@ptupitsyn
Copy link
Contributor

General note on async APIs

Currently, the APIs are based on std::future, which is very limited, unfortunately:

  • Not possible to get a completion callback.
  • Not possible to co_await.

Basically, we can only perform a blocking wait or some kind of busy loop wait.

I think a callback-based API is a better alternative (user can pass a callback that will be invoked upon completion).

  • Can be used directly without any libraries or newer language features and achieve non-blocking behavior.
  • Can we converted to other async constructs, like std::future, boost::future, awaitable (for co_await in C++ 20).

Example: Asio library with callbacks, futures, coroutines (awaitables)

Comment on lines +75 to +77
auto pool = m_pool;
if (pool)
pool->stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auto pool = m_pool;
if (pool)
pool->stop();
if (auto pool = m_pool)
pool->stop();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Come on, this is not an error :)

Copy link
Contributor

@ademakov ademakov Sep 28, 2022

Choose a reason for hiding this comment

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

yes, just a suggestion, you may ignore it

return;
}
else if (it != m_connections.end())
connection = it->second;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reaction if the connection not found? Perhaps write something to log at least?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a normal situation actually. Connection could be closed before we handle a received message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Perhaps write a comment?

else if (it != m_connections.end())
connection = it->second;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes 2 locks on every message. This is not going to scale well. Especially, if I got it correct, considering that after the handshake is done, for all of the following messages these locks are practically useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I'll try to fix it.

* @return TableImpl with corresponding name.
* @throw IgniteError In case of error.
*/
void getTableAsync(const std::string& name, IgniteCallback<std::optional<Table>> callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to have more methods here? And more fields? Currently this class looks excessive. We can move this method (and any methods like this that use only m_connection) directly to ClusterConnection.

Copy link
Contributor Author

@isapego isapego Sep 28, 2022

Choose a reason for hiding this comment

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

Sure. This is an entry point for table management API. There are going to be such methods as createTable, updateTable, dropTable, etc. Currently there are very few methods in user API overall but thats just to not inflate PR which is big enough already.

Copy link
Contributor

@ademakov ademakov left a comment

Choose a reason for hiding this comment

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

LGTM

@isapego isapego merged commit c685b09 into apache:main Sep 28, 2022
@isapego isapego deleted the ignite-17424 branch September 28, 2022 16:33
lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Mar 18, 2023
lowka pushed a commit to gridgain/apache-ignite-3 that referenced this pull request Apr 19, 2023
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.

3 participants