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

Implement and test transaction timeouts #364

Merged
merged 8 commits into from
Dec 20, 2021

Conversation

flyingsilverfin
Copy link
Member

@flyingsilverfin flyingsilverfin commented Dec 14, 2021

What is the goal of this PR?

We implement options for transaction timeout configuration (typedb/typedb#6487). We also add BDD implementation for new steps required to test transaction timeout and configuring transaction options.

What are the changes implemented in this PR?

  • Add option for transaction timeout (default: 5 mins)
  • fix build by adding missing maven dependencies from common
  • fix a bug where errors submitted to the response queue were ignored (cleared immediately) - these are now drained from the response queue
  • add missing BDD steps for session and transaction option configuration

@flyingsilverfin flyingsilverfin changed the title Add tx timeout option, bdd test impl, and drain errors held in client Add and test transaction timeouts Dec 14, 2021
@flyingsilverfin flyingsilverfin marked this pull request as ready for review December 15, 2021 15:23
@@ -126,6 +126,7 @@ vaticle_typedb_artifact()
vaticle_typedb_cluster_artifact()

# Load maven
load("@vaticle_typedb_common//dependencies/maven:artifacts.bzl", vaticle_typedb_common_artifacts = "artifacts")
Copy link
Member Author

Choose a reason for hiding this comment

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

we need to load common maven requirements as well

Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

because we depend on all of common, and common now has a sub-library (without its own package) that requires maven dependencies!

api/TypeDBOptions.java Show resolved Hide resolved
api/TypeDBOptions.java Show resolved Hide resolved
common/exception/ErrorMessage.java Outdated Show resolved Hide resolved
connection/TypeDBTransactionImpl.java Outdated Show resolved Hide resolved
stream/ResponseCollector.java Show resolved Hide resolved
Comment on lines 59 to 67
public static TypeDBOptions sessionOptions;
public static TypeDBOptions transactionOptions;
static boolean isBeforeAllRan = false;

public static final Map<String, BiConsumer<TypeDBOptions, Integer>> optionSetters = map(
pair("session-idle-timeout-millis", TypeDBOptions::sessionIdleTimeoutMillis),
pair("transaction-timeout-millis", TypeDBOptions::transactionTimeoutMillis)
);
Copy link
Member Author

Choose a reason for hiding this comment

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

we set up only the initial options we need to configure

@@ -126,6 +126,7 @@ vaticle_typedb_artifact()
vaticle_typedb_cluster_artifact()

# Load maven
load("@vaticle_typedb_common//dependencies/maven:artifacts.bzl", vaticle_typedb_common_artifacts = "artifacts")
Copy link
Member

Choose a reason for hiding this comment

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

why?

connection/TypeDBTransactionImpl.java Outdated Show resolved Hide resolved
dependencies/maven/artifacts.snapshot Outdated Show resolved Hide resolved
@@ -45,6 +47,7 @@ void beforeAll() {
}
server.start();
TypeDBSingleton.setTypeDBRunner(server);
ConnectionStepsBase.isBeforeAllRan = true;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary, right? Since the implementation of before in ConnectionStepsBase already sets isBeforeAllRan to true,

(having said that, isBeforeAllRan being set in a method that isn't beforeAll feels pretty dodgy, so perhaps there is a better solution 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.

cleaned it up

Copy link
Member Author

Choose a reason for hiding this comment

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

Urgh actually im going to revert this, and when we upgrade Cucumber we can use a new version that has @BeforeAll and @AfterAll

test/behaviour/connection/ConnectionStepsCore.java Outdated Show resolved Hide resolved
@@ -114,4 +131,8 @@ void connection_does_not_have_any_database() {
assertNotNull(client);
assertTrue(client.isOpen());
}

void wait_seconds(int seconds) throws InterruptedException {
Copy link
Member

Choose a reason for hiding this comment

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

Feels like this should be in some kind of UtilSteps.. but then it'd be all alone, so ¯\_(ツ)_/¯

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually let me do that, it'll probably grow

@@ -152,4 +155,12 @@ public void sessions_in_parallel_have_databases(List<String> names) {

CompletableFuture.allOf(assertions).join();
}

@Given("set session option {word} to: {int}")
public void set_session_option_to(String option, int value) {
Copy link
Member

Choose a reason for hiding this comment

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

We have options that are not int-valued. Even though we aren't testing them yet, I imagine we'll do so soon, so we should future-proof this step by renaming it to

@Given("set session option {word} to int: {int}")

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented this by always using strings and parsing them in the option setters

stream/BidirectionalStream.java Outdated Show resolved Hide resolved
stream/ResponseCollector.java Outdated Show resolved Hide resolved
@flyingsilverfin flyingsilverfin merged commit fc60cd5 into typedb:master Dec 20, 2021
@flyingsilverfin flyingsilverfin deleted the tx-timeout-option branch December 20, 2021 17:46
@flyingsilverfin flyingsilverfin changed the title Add and test transaction timeouts Implement and test transaction timeouts Dec 21, 2021
flyingsilverfin added a commit to typedb/typedb-console that referenced this pull request Jan 5, 2022
## What is the goal of this PR?

Add transaction timeouts to the console options available. This allows users to manipulate the transaction timeout added in typedb/typedb-driver#364.

## What are the changes implemented in this PR?

* add extra option for transaction timeout
* bump version to 2.6.1 to prepare for patch release
lolski pushed a commit that referenced this pull request Mar 14, 2022
## What is the goal of this PR?

We revert previous changes from #368 and #364, which made query queues and iterators throw the same error idempotently if there was one. However, this goes counter to standard usages of iterators and queues, which are not meant to behave idempotently (each item is only returned once, and if they have an error they should no longer be used). 

## What are the changes implemented in this PR?

* remove idempotent error state of collectors and queues, which back query iterators
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants