-
Notifications
You must be signed in to change notification settings - Fork 5
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
Reindexer context support #20
base: master
Are you sure you want to change the base?
Conversation
… into feature/add_transaction_support
…s' into feature/add_transaction_support
# Conflicts: # pyreindexer/lib/include/queryresults_wrapper.h # pyreindexer/lib/src/rawpyreindexer.cc # pyreindexer/lib/src/rawpyreindexer.h
# Conflicts: # README.md # pyreindexer/example/main.py # pyreindexer/lib/include/queryresults_wrapper.h # pyreindexer/lib/include/transaction_wrapper.h # pyreindexer/lib/src/rawpyreindexer.cc # pyreindexer/lib/src/rawpyreindexer.h # pyreindexer/lib/src/reindexerinterface.cc # pyreindexer/lib/src/reindexerinterface.h # pyreindexer/query_results.py # pyreindexer/raiser_mixin.py # pyreindexer/rx_connector.py # pyreindexer/tests/tests/test_transaction.py # pyreindexer/transaction.py # readmegen.sh
…g_support # Conflicts: # pyreindexer/example/main.py # pyreindexer/lib/src/reindexerinterface.h # pyreindexer/rx_connector.py
…xt_support # Conflicts: # pyreindexer/example/main.py # pyreindexer/lib/src/reindexerinterface.cc # pyreindexer/lib/src/reindexerinterface.h # pyreindexer/rx_connector.py
# Conflicts: # README.md # pyreindexer/example/main.py # pyreindexer/lib/include/pyobjtools.cc # pyreindexer/lib/include/pyobjtools.h # pyreindexer/lib/include/query_wrapper.cc # pyreindexer/lib/include/query_wrapper.h # pyreindexer/lib/include/transaction_wrapper.h # pyreindexer/lib/src/rawpyreindexer.cc # pyreindexer/lib/src/rawpyreindexer.h # pyreindexer/lib/src/reindexerinterface.cc # pyreindexer/lib/src/reindexerinterface.h # pyreindexer/rx_connector.py # pyreindexer/transaction.py
…r_context_support # Conflicts: # README.md # pyreindexer/lib/src/reindexerinterface.cc # pyreindexer/lib/src/reindexerinterface.h # pyreindexer/rx_connector.py # pyreindexer/transaction.py
# Conflicts: # pyreindexer/lib/src/rawpyreindexer.cc # pyreindexer/lib/src/reindexerinterface.cc # pyreindexer/lib/src/reindexerinterface.h # pyreindexer/rx_connector.py
### RxConnector.with\_timeout | ||
|
||
```python | ||
def with_timeout(timeout: int) -> RxConnector |
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.
Предложение 'на подумать': в питоне есть тип datetime.timedelta, который здесь можно было бы принимать вместо int. Он более выразительный, потому что там явно видно, в каких единицах измерения задан таймаут. Но при этом и заметно более многословный, поэтому настаивать не буду
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.
Показательность это хорошо. Но когда требуется не ради функционала добавить зависимость и привязаться к ещё одному модулю. Я стараюсь быть осторожным и не делать резких движений. Добавим, ...если что открутим, но в общем случае - я против.
|
||
def select_all_item_query_example(db, namespace): | ||
return db.select("SELECT * FROM " + namespace) | ||
return db.with_timeout(1000).select("SELECT * FROM " + namespace) |
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.
Я бы предложил добавить ещё пример с query-билдером+таймаут и транзакция+таймаут. Лично мне кажется немного неинтуитивным то, что там нужно задавать таймаут перед созданием запроса, а не в момент его выполнения, и поэтому будет лучше это подсветить в примере
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.
С примерами у нас беда. Его нужно вдумчиво переработать. У меня это записано.
Мне сам концепт не нравится, но сделано по аналогии с остальными коннекторами и reindexer. Наиболее очевидное решение передавать параметром в те функции которые его реально используют. Тем более, что от контекста остался только timeout
template <typename DBT> | ||
typename DBT::TransactionT ReindexerInterface<DBT>::startTransaction(std::string_view ns) { | ||
auto transaction = db_.WithTimeout(timeout_).NewTransaction(ns); | ||
timeout_ = std::chrono::milliseconds{0}; |
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.
Насколько я вижу, такой подход сейчас не позволяет применить таймаут к коммиту транзакции. Таймаут, который передал пользователь, в момент вызова new_transaction будет сброшен здесь, а новый таймаут передать в объект транзакции уже нельзя (либо я чего-то не увидел).
И ещё, кажется, что это будет плохо работать в ситуациях, когда у пользователя между созданием запроса через new_query и его выполнение есть какие-то другие команды. Например:
query = rx.with_timeout(100).new_query(namespace).where('id', CondType.CondEq, 1)
rx.with_timeout(1000).upsert('{ "id": 1 }')
result = query.must_execute
В таком коде, кажется, таймаут 100 вообще не будет использован, а query выполнится без таймаута.
То есть как будто QueryWrapper и TransactionWrapper должны иметь информацию о своих таймаутах
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.
То есть как будто QueryWrapper и TransactionWrapper должны иметь информацию о своих таймаутах
Это сделать можно. Мне вообще не нравится скопированный комментарий - Optional server-side execution timeout for each subquery. На деле timeout одноразовый, и мне даже это кажется правильным. Он нужен\важен не каждому запросу.
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.
template <typename DBT>
typename DBT::TransactionT ReindexerInterface<DBT>::startTransaction(std::string_view ns) {
auto transaction = db_.WithTimeout(timeout_).NewTransaction(ns);
timeout_ = std::chrono::milliseconds{0};
return transaction;
}
template <typename DBT>
Error ReindexerInterface<DBT>::commitTransaction(typename DBT::TransactionT& transaction, size_t& count) {
typename DBT::QueryResultsT qres(QRESULTS_FLAGS);
auto err = db_.WithTimeout(timeout_).CommitTransaction(transaction, qres);
timeout_ = std::chrono::milliseconds{0};
count = qres.Count();
return err;
}
template <typename DBT>
Error ReindexerInterface<DBT>::rollbackTransaction(typename DBT::TransactionT& transaction) {
auto err = db_.WithTimeout(timeout_).RollBackTransaction(transaction);
timeout_ = std::chrono::milliseconds{0};
return err;
}
А значит, можно например так:
# start transaction
transaction = db.with_timeout(50).new_transaction(namespace)
# delete first few items
...
# update last one item, overwrite field 'value'
...
# stop transaction and commit changes to namespace
transaction.with_timeout(100).commit()
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.
И ещё, кажется, что это будет плохо работать в ситуациях, когда у пользователя между созданием запроса через new_query и его выполнение есть какие-то другие команды
Заблуждение. Timeout срабатывает только при выходе в сеть. Поэтому составление запроса по средством QueryBuilder не теребит timeout. Да, для new_query
он не используется, мы никуда не обращаемся
template <typename DBT>
Error ReindexerInterface<DBT>::selectQuery(const Query& query, QueryResultsWrapper& result) {
typename DBT::QueryResultsT qres(QRESULTS_FLAGS);
auto err = db_.WithTimeout(timeout_).Select(query, qres);
timeout_ = std::chrono::milliseconds{0};
result.Wrap(std::move(qres));
return err;
}
template <typename DBT>
Error ReindexerInterface<DBT>::deleteQuery(const Query& query, size_t& count) {
typename DBT::QueryResultsT qres;
auto err = db_.WithTimeout(timeout_).Delete(query, qres);
timeout_ = std::chrono::milliseconds{0};
count = qres.Count();
return err;
}
template <typename DBT>
Error ReindexerInterface<DBT>::updateQuery(const Query& query, QueryResultsWrapper& result) {
typename DBT::QueryResultsT qres(QRESULTS_FLAGS);
auto err = db_.WithTimeout(timeout_).Update(query, qres);
timeout_ = std::chrono::milliseconds{0};
result.Wrap(std::move(qres));
return err;
}
Для Query актуальны только эти методы - execute\delete\update\must_execute
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.
query = rx.with_timeout(100).new_query(namespace).where('id', CondType.CondEq, 1)
rx.with_timeout(1000).upsert('{ "id": 1 }')
result = query.must_execute
query = rx.with_timeout(100).new_query(
в холостую, не используетсяrx.with_timeout(1000).upsert('{ "id": 1 }')
вызов с timeoutresult = query.must_execute
вызов без timeout, да он затёрся, нужен timeout зовите явноresult = query.with_timeout(1000).must_execute
очевидно ли такое поведение - очевидно нет. Но если пользователю сказать, что перед длительным запросом, он может задать timeout непосредственно перед вызовом этой операции. Это воспринимается нормально.
можно ли сделать, чтобы каждый кто использует timeout хранил своё значение - можно, всё равно будут вопросики он одноразовый или на все запросы распространяется. И да таких классов 3: rx_connector, transaction, query.
Мне кажется, ещё один параметр (опциональный) в каждый метод для которого это актуально - более правильный путь
От всего контекста на данный момент остался только WithTimeout()