8000 Feature/refactor arangobench by cpjulia · Pull Request #14548 · arangodb/arangodb · GitHub
[go: up one dir, main page]
Skip to content

Feature/refactor arangobench #14548

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.

Sign up for GitHub

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

Merged
merged 40 commits into from
Aug 4, 2021
Merged

Feature/refactor arangobench #14548

Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
80536ea
Started refactoring with Velocypack
cpjulia Jul 21, 2021
4abf968
Test for account email change
cpjulia Jul 22, 2021
008cea9
Integrated Velocypack to document-import and removed comments that we…
cpjulia Jul 22, 2021
9a7c7c2
Integrated arangobench to Velocypack
cpjulia Jul 27, 2021
054662f
Merge branch 'devel' of github.com:arangodb/arangodb into feature/ref…
cpjulia Jul 27, 2021
e9910cd
Fixed compilation bug by adding const to the payload function definit…
cpjulia Jul 27, 2021
0638ac9
Merge branch 'devel' of github.com:arangodb/arangodb into feature/ref…
cpjulia Jul 27, 2021
515e159
Removed unnecessary indentation from file
cpjulia Jul 27, 2021
335414a
Fixed typos in document testkeys
cpjulia Jul 27, 2021
e32bbae
Apply suggestions from code review
cpjulia Jul 28, 2021
92383c8
Applied suggested changes on review
cpjulia Jul 28, 2021
a01a3b2
Merge branch 'feature/refactor-arangobench' of github.com:arangodb/ar…
cpjulia Jul 28, 2021
68be152
Merge branch 'devel' of github.com:arangodb/arangodb into feature/ref…
cpjulia Jul 28, 2021
dc6eb5c
Added log topic BENCH
cpjulia Jul 28, 2021
79a35f4
Merge branch 'devel' of github.com:arangodb/arangodb into feature/ref…
cpjulia Jul 28, 2021
61a108d
Merged url, type and payload functions into one, changed type of LOG …
cpjulia Jul 29, 2021
38dca35
Merge branch 'devel' of github.com:arangodb/arangodb into feature/ref…
jsteemann Jul 29, 2021
f2ff275
Fixes for results calculations
jsteemann Jul 29, 2021
b113e67
simplify some test cases
jsteemann Jul 30, 2021
14e930b
slightly clean up API
jsteemann Jul 30, 2021
436a6f0
simplify request handling
jsteemann Jul 30, 2021
11b44d3
simplify test cases
jsteemann Jul 30, 2021
03aeb2c
simplify test cases
jsteemann Jul 30, 2021
07ee251
simplify test cases
jsteemann Jul 30, 2021
630603a
Removed escape for character % within a string that was causing compi…
cpjulia Jul 30, 2021
b8d61a3
Merge branch 'devel' of github.com:arangodb/arangodb into feature/ref…
cpjulia Jul 30, 2021
91b147b
reduce formatting differences in main files
jsteemann Aug 2, 2021
03f57f4
micro optimizations
jsteemann Aug 2, 2021
b42c2d5
set some velocypack Options for Builder
jsteemann Aug 2, 2021
8b4b981
allow recycling of SimpleHttpResult objects
jsteemann Aug 2, 2021
23db23c
add "bench" log topic
jsteemann Aug 2, 2021
760b2b4
unrolled case-insensitive comparison
jsteemann Aug 3, 2021
b0c136a
Merge branch 'devel' of github.com:arangodb/arangodb into feature/ref…
jsteemann Aug 3, 2021
00a6f43
Merge branch 'devel' of github.com:arangodb/arangodb into feature/ref…
cpjulia Aug 3, 2021
c948826
CHANGELOG
cpjulia Aug 3, 2021
07ce765
fix gtest
jsteemann Aug 3, 2021
704dad5
Update arangosh/Benchmark/testcases/AqlInsertTest.h
jsteemann Aug 4, 2021
3c90697
address review comments
jsteemann Aug 4, 2021
f95b976
fix error counters for batching
jsteemann Aug 4, 2021
2ecf023
fix some out-of-range casts reported by ASan
jsteemann Aug 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix error counters for batching
  • Loading branch information
@jsteemann
jsteemann committed Aug 4, 2021
commit f95b9766fcbaf6694329d6b85e9c128a42bd265f
11 changes: 6 additions & 5 deletions arangosh/Benchmark/BenchmarkThread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ class BenchmarkThread : public arangodb::Thread {
double delta = TRI_microtime() - start;
trackTime(delta);

processResponse(result.get(), /*batch*/ true);
processResponse(result.get(), /*batch*/ true, numOperations);

_httpClient->recycleResult(std::move(result));
}
Expand Down Expand Up @@ -341,13 +341,14 @@ class BenchmarkThread : public arangodb::Thread {
double delta = TRI_microtime() - start;
trackTime(delta);

processResponse(result.get(), /*batch*/ false);
processResponse(result.get(), /*batch*/ false, 1);

_httpClient->recycleResult(std::move(result));
}

void processResponse(httpclient::SimpleHttpResult const* result, bool batch) {
void processResponse(httpclient::SimpleHttpResult const* result, bool batch, uint64_t numOperations) {
char const* type = (batch ? "batch" : "single");
TRI_ASSERT(numOperations > 0);

if (result != nullptr &&
result->isComplete() &&
Expand All @@ -371,9 +372,9 @@ class BenchmarkThread : public arangodb::Thread {
return;
Copy link
Contributor
@mpoeter mpoeter Aug 4, 2021

Choose a reason for hiding this comment

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

If result != nullptr && result->isComplete() && !result->wasHttpError() && !batch is satisfied we will return here without updating any counters - this is a change to be previous behavior.

Copy link
Contributor
@jsteemann jsteemann Aug 4, 2021

Choose a reason for hiding this comment

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

I think the change is valid, because counters should only increased when there are any errors (the counters are simply error counters).

For non-batch requests (i.e. !batch) an error was previously assumed if result == nullptr || !result->isComplete() || result->wasHttpError().
In the refactored version, we are still counting in this case.

And for batch requests (.e. batch) this case should also be ok, because for complete HTTP responses the number of errors is only in some header, which we are still checking.

And in case result == nullptr || !result->isComplete() || result->wasHttpError() we are still counting everything, regardless of batch requests or single requests.

Copy link
Contributor
@jsteemann jsteemann Aug 4, 2021

Choose a reason for hiding this comment

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

Btw in the previous code we had two different implementations for the error handling. One in executeSingleRequest and executeBatchRequest. We tried to put them together here, which made the if conditions a bit more complex, but still should reduce code size.

Copy link
Contributor
@mpoeter mpoeter Aug 4, 2021

Choose a reason for hiding this comment

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

Ah, so basically a batch request should never report an HTTP error?
I saw that you tried to unify the error handling for the single/batch requests, but unfortunately I still t 6D65 hink this function does not behave like the previous code. For example, for batch requests the old code increased _operationsCounter by numOperations while now we always increment by one. Also, the old code for batch requests had an optional verbose output that is no longer available in the new code.
Can we have batch requests with a single operation? If not, I would replace the batch argument with a numOperations argument and derive bool batch = numOperations > 1.

}

_operationsCounter->incFailures(1);
_operationsCounter->incFailures(numOperations);
if (result != nullptr && !result->isComplete()) {
_operationsCounter->incIncompleteFailures(1);
_operationsCounter->incIncompleteFailures(numOperations);
}
if (++_warningCount < maxWarnings) {
if (result != nullptr && result->wasHttpError()) {
Expand Down
0