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

bench: cluster patch #752

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kirill-Churkin
Copy link
Contributor

@Kirill-Churkin Kirill-Churkin commented Jan 16, 2023

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

Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

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

@@ -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/)
Copy link
Member

Choose a reason for hiding this comment

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

Please, rebase on master and resolve changelog conflict.

### Added

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

Choose a reason for hiding this comment

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

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,


- 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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

"github.com/tarantool/cartridge-cli/cli/context"
)

// verifyLeaders check each replica have leader.
Copy link
Member

Choose a reason for hiding this comment

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

The method is verifyReplicas.


### Added

- Tarantool benchmark tool update (cluster bench):
Copy link
Member

Choose a reason for hiding this comment

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

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?

return nil
}

// createNodesConnectionsPools creates connections pool for every node in cluster.
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -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/)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have tests for the feature.


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

Choose a reason for hiding this comment

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

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

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

// RotaryConnectionsPool describes round-cycled connection pool.
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

@DifferentialOrange DifferentialOrange Feb 7, 2023

Choose a reason for hiding this comment

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

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
Copy link

Mons commented Feb 21, 2023

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

@LeonidVas LeonidVas removed their request for review March 23, 2023 15:02
Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

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

@sergos
Copy link

sergos commented Jan 26, 2024

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

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.

5 participants