bench: cluster patch #752

Kirill-Churkin · 2023-01-16T15:39:29Z

user@cartridge-cli % ./cartridge bench --leader=127.0.0.1:3301 --replica=127.0.0.1:3302
Tarantool 2.10.2 (Binary) b6820873-f007-4413-b2f0-69fe833c0e14 

Parameters:
        Leaders:
                127.0.0.1:3301
        Replicas:
                127.0.0.1:3302
        user: guest
        connections: 10
        simultaneous requests: 10
        duration: 10 seconds
        key size: 10 bytes
        data size: 20 bytes
        insert: 100 percentages
        select: 0 percentages
        update: 0 percentages

Data schema
| key                            | value
| ------------------------------ | ------------------------------
| random(10)                     | random(20)
Benchmark start
...
Benchmark stop.

Results:
        Success operations: 760386
        Failed  operations: 0
        Request count: 760474
        Time (seconds): 9.979464
        Requests per second: 76203

DifferentialOrange

Thank you for your contribution! Seems legit at most, I have left a few comments.

DifferentialOrange · 2023-02-07T07:33:13Z

CHANGELOG.md

@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)


Please, rebase on master and resolve changelog conflict.

DifferentialOrange · 2023-02-07T07:43:08Z

CHANGELOG.md

+### Added
+
+- Tarantool benchmark tool update (cluster bench):
+  * option --leader has been added - sest array of url's for leaders.


Suggested change

* option --leader has been added - sest array of url's for leaders.

* option --leader has been added - set array of url's for leaders,

DifferentialOrange · 2023-02-07T07:43:19Z

CHANGELOG.md

+
+- Tarantool benchmark tool update (cluster bench):
+  * option --leader has been added - sest array of url's for leaders.
+  * option --replica has been added - sest array of url's for replicas.


Suggested change

* option --replica has been added - sest array of url's for replicas.

* option --replica has been added - set array of url's for replicas.

DifferentialOrange · 2023-02-07T07:55:31Z

cli/bench/cluster.go

+	"github.com/tarantool/cartridge-cli/cli/context"
+)
+
+// verifyLeaders check each replica have leader.


The method is verifyReplicas.

DifferentialOrange · 2023-02-07T08:05:03Z

CHANGELOG.md

+
+### Added
+
+- Tarantool benchmark tool update (cluster bench):


To be honest, I don't get a full understanding of what "cluster" is here.

It seems that one of the options for a cluster is a replicaset. No questions here.

But it's more complicated with a vshard cluster. Yeah, each storage in a vshard cluster has the same schema (if we're talking about the same vshard group), so you theoretically can send the same CRUD requests to each storage in a vshard cluster. But you never work with vshard storages directly, you use a router (especially since each record from a sharded space should have a bucket id, and it's computed on a router). So it doesn't make much sense to bench storages only, especially since stored procedures on a router may be complicated and the bottleneck of the vshard clusters is often the network requests between a router and a storage.

So the question is as follows. Is "cluster bench" actually a "replicaset bench"? What did you use it for?

DifferentialOrange · 2023-02-07T08:39:03Z

cli/bench/cluster.go

+	return nil
+}
+
+// createNodesConnectionsPools creates connections pool for every node in cluster.


Description is confusing. At first I have thought that we have not two, but N connection pool with a single node in each of them.

DifferentialOrange · 2023-02-07T08:56:19Z

CHANGELOG.md

@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)


I think we should have tests for the feature.

DifferentialOrange · 2023-02-07T08:57:42Z

cli/bench/types.go

+
+// RotaryNodesConnectionsPools describes round-cycled cluster nodes array,
+// where each represented by RotaryConnectionsPool.
+type RotaryNodesConnectionsPools struct {


I don't get what's the motivation behind such complicated hierarchy.

DifferentialOrange · 2023-02-07T09:00:43Z

cli/bench/types.go

@@ -16,15 +18,31 @@ type Results struct {
 	requestsPerSecond    int     // Cumber of requests per second - the main measured value.
 }

+// RotaryConnectionsPool describes round-cycled connection pool.


For the future, I think you may consider developing bench as tt extension. tt uses tarantool/go-tarantool as a connector, and go-tarantool has built-in support of connection pools. It is also worth to mention that you should be able to develop an extention within your own repo.

DifferentialOrange · 2023-02-07T09:46:05Z

cli/bench/cluster.go

+		benchData.waitGroup.Add(1)
+		go func() {
+			defer benchData.waitGroup.Done()
+			requestsSequence := RequestsSequence{


Now requests generation is the part of the bench run tracked by timer. I think it's better to build request data before starting the timer, if possible.

Mons · 2023-02-21T14:52:46Z

This should be better in tt, rather than in cartridge-cli

DifferentialOrange

Just a status clarification, since GitHub treats this as "need review"

sergos · 2024-01-26T08:02:04Z

@LeonidVas please consider opportunity to transfer/adapt the test to tt perf bench.

bench: cluster patch

aea0db9

Kirill-Churkin force-pushed the tarantool-benchmark branch from 6172ec4 to aea0db9 Compare January 16, 2023 15:46

LeonidVas self-requested a review January 16, 2023 15:55

LeonidVas requested a review from DifferentialOrange February 6, 2023 10:43

DifferentialOrange reviewed Feb 7, 2023

View reviewed changes

LeonidVas removed their request for review March 23, 2023 15:02

DifferentialOrange requested changes Apr 19, 2023

View reviewed changes

sergos assigned LeonidVas Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bench: cluster patch #752

bench: cluster patch #752

Kirill-Churkin commented Jan 16, 2023 •

edited

DifferentialOrange left a comment

DifferentialOrange Feb 7, 2023

DifferentialOrange Feb 7, 2023

DifferentialOrange Feb 7, 2023

DifferentialOrange Feb 7, 2023

DifferentialOrange Feb 7, 2023

DifferentialOrange Feb 7, 2023

DifferentialOrange Feb 7, 2023

DifferentialOrange Feb 7, 2023

DifferentialOrange Feb 7, 2023

DifferentialOrange Feb 7, 2023 •

edited

Mons commented Feb 21, 2023

DifferentialOrange left a comment •

edited

sergos commented Jan 26, 2024

		@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
		The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)

	* option --leader has been added - sest array of url's for leaders.
	* option --leader has been added - set array of url's for leaders,

	* option --replica has been added - sest array of url's for replicas.
	* option --replica has been added - set array of url's for replicas.

bench: cluster patch #752

Are you sure you want to change the base?

bench: cluster patch #752

Conversation

Kirill-Churkin commented Jan 16, 2023 • edited

DifferentialOrange left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DifferentialOrange Feb 7, 2023 • edited

Choose a reason for hiding this comment

Mons commented Feb 21, 2023

DifferentialOrange left a comment • edited

Choose a reason for hiding this comment

sergos commented Jan 26, 2024

Kirill-Churkin commented Jan 16, 2023 •

edited

DifferentialOrange Feb 7, 2023 •

edited

DifferentialOrange left a comment •

edited