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

Initial New Client #59

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Initial New Client #59

wants to merge 15 commits into from

Conversation

umgefahren
Copy link
Owner

This marks the start of implementing a new client.
This pull request doesn't really contain any new value, it will take time until this is finished, but it implements the requested interactive mode.

@umgefahren umgefahren added enhancement New feature or request Client Regarding the benchmark client labels Apr 9, 2022
@umgefahren umgefahren self-assigned this Apr 9, 2022
@umgefahren umgefahren linked an issue Apr 9, 2022 that may be closed by this pull request
@umgefahren umgefahren marked this pull request as draft April 9, 2022 16:48
@umgefahren umgefahren marked this pull request as ready for review April 9, 2022 19:20
@Zollerboy1 Zollerboy1 self-requested a review April 9, 2022 19:51
Copy link
Collaborator

@Zollerboy1 Zollerboy1 left a comment

Choose a reason for hiding this comment

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

Added some comments

@@ -0,0 +1 @@
advanced-client
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also add plan.yaml in here

Comment on lines 22 to 37
var generateDataOpt = flag.Bool("gen", false, "Pass this flag to generate benchmark data")
var performBenchmarkOpt = flag.Bool("bench", false, "Pass this flag to perform a benchmark")
var performTestOpt = flag.Bool("test", false, "Pass this flag to perform a test")
var interactiveModeOpt = flag.Bool("it", false, "Pass this flag to run in interactive mode")
var singleModeOpt = flag.Bool("single", false, "Pass this flag to run a single command")
var helpOpt = flag.Bool("h", false, "Pass this flag to get help")
var serverHostnameOpt = flag.String("host", "localhost", "Hostname of server for benchmark/test/interactive/single")
var serverPortOpt = flag.Uint("port", 8080, "Port of server for benchmark/test/interactive/single")
var testLevelOpt = flag.Uint("level", 1, "Command complexity level for test (0 -> Only separate specified ; 1 -> Essential ; 2 -> 1 + Dumping ; 3 -> 2 + Delayed ; 4 -> 3 + Heavy Load)")
var commandOpt = flag.String("cmd", "all", "Command to perform in single mode or comma seperated commands for testing")
var testCyclesOpt = flag.Int("c", 10, "Specify how often the test cycle should be performed")
var keyOpt = flag.String("key", "hello", "Key value for single mode")
var valueOpt = flag.String("value", "world", "Value value for single mode")
var durationOpt = flag.Duration("duration", time.Second*10, "Duration for single mode")
var outOpt = flag.String("o", "bench.txt", "Path for output of benchmark/data generation")
var amountOpt = flag.Float64("amount", 0.5, "Amount of data in GB to generate")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just comment out all the flags that aren't implemented at the moment. Also, the 'operation' options (like -gen, -test, etc.) should be subcommands rather than flags.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I could do that, sadly it's not as easy as with Rust's clap, so I would prefer to leave the operation as is. The unimplemented flags will be commented out in the next comment.

ret.hostname = *serverHostnameOpt
ret.port = *serverPortOpt
default:
panic("WTF?")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, this gets called when you pass the -single flag. It should probably be handled in a separate switch branch.

@TecTrixer
Copy link
Collaborator

I don't have time to review so much code rn. I'll do it tomorrow and simply test it

@vypxl
Copy link
Collaborator

vypxl commented Apr 11, 2022

May I add a commit where I rewrote limits.go to use syscalls instead of executing ulimit? The current method does not work on linux as ulimit is a shell builtin and not an executable. Also, syscalls should be preferred imo.

@umgefahren
Copy link
Owner Author

You are right, I will change this

@vypxl
Copy link
Collaborator

vypxl commented Apr 11, 2022

I felt free to implement the changes I would have requested:

  • limits are read and set via syscalls
  • cli uses subcommands instead of a ton of obscure options
  • color output with background colors is unreadable

I will evaluate the rest of the code some time later.

@Zollerboy1 Zollerboy1 marked this pull request as draft April 28, 2022 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client Regarding the benchmark client enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

New Client
4 participants