Change get_standing_orders_of_slot to return always all standing_orders, including non-placed ones #155

josojo · 2019-06-21T05:45:57Z

tackels #152

testplan:
unit tests only

… entries

driver/src/models/standing_order.rs

josojo · 2019-06-21T09:34:25Z

driver/src/db_interface.rs

+        );
+        let mut standing_orders = vec![empty_standing_order; models::NUM_RESERVED_ACCOUNTS as usize];
+        for standing_order in non_zero_standing_orders {
+            standing_orders[standing_order.account_id as usize] = standing_order.clone();


I tried to be less clonely(), but did not find a way to do it. I have seen people switching to references or using std::mem::swap. But I think neither of these methods is appropriated here.

Does foreach work (https://docs.rs/foreach/0.3.0/foreach/)?

ForEach trait and for_each! macro allow you to use iterator inside iteration loop, which is not posible when using for-in loop.

I use for_each now. It makes the code easier to understand. However, I was not able to remove the clone.

fleupold

I think we should populate the accountId correctly even in the fake records.

Also could get_standing_orders_of_slot now return a fixed length array of size NUM_RESERVED_ACCOUNTS?

driver/src/db_interface.rs

fleupold · 2019-06-21T13:48:18Z

driver/src/db_interface.rs

+        );
+        let mut standing_orders = vec![empty_standing_order; models::NUM_RESERVED_ACCOUNTS as usize];
+        for standing_order in non_zero_standing_orders {
+            standing_orders[standing_order.account_id as usize] = standing_order.clone();


Does foreach work (https://docs.rs/foreach/0.3.0/foreach/)?

ForEach trait and for_each! macro allow you to use iterator inside iteration loop, which is not posible when using for-in loop.

driver/src/models/standing_order.rs

driver/src/order_driver.rs

bh2smith

This is starting to look very "Rustic"!

fleupold

What were your thoughts around changing the return type of get_standing_orders_of_slot to Result<[models::StandingOrder; models::NUM_RESERVED_ACCOUNTS as usize], DriverError>; to make clear that it will always contain the full list augmented with empty StandingOrders?

driver/src/db_interface.rs

bh2smith · 2019-06-26T12:41:00Z

Tests appear to be failing here because of maximum retires:

waited 181 seconds
Before retry #1: sleeping 0.3 seconds
Before retry #2: sleeping 0.6 seconds
Before retry #3: sleeping 1.2 seconds
Before retry #4: sleeping 2.4 seconds
Before retry #5: sleeping 4.8 seconds
Retries exhausted
The command "sh ./test/e2e-tests-standing-order.sh" exited with 1.

Maybe it is a good time for us to try and fix this.

josojo · 2019-06-27T08:32:58Z

I figured out that using :
Result<[models::StandingOrder; models::NUM_RESERVED_ACCOUNTS as usize], DriverError>;
is not a wise, as we can not differentiate nullified orders ( orders with just zeros as parameters) and non-placed orders at all. But for the rolling hash caclulation, it actually does make a difference, whether we have a non-placed order or a zero order.
Differentiating these two cases would require the introduction of a counter of orders placed. I think this is unjustified complexity. I will revert the latest changes on this PR.

fleupold · 2019-07-01T10:17:42Z

But for the rolling hash caclulation, it actually does make a difference, whether we have a non-placed order or a zero order.

You are right. I wonder if this is something we need to change in the future as in the snark we will likely fill up unoccupied slots with 0s.

josojo · 2019-07-01T15:25:09Z

You are right. I wonder if this is something we need to change in the future as in the snark we will likely fill up unoccupied slots with 0s.

Yeah, then many things might be needed to change. Also, our rolling hash of the normal orders would be required to be computed differently. It would always need to be a rolling hash of 500 orders, just x<500 would no longer be an option, as the snark would also always compute 500 rolling hashes.

josojo · 2019-07-03T08:54:02Z

@fleupold I think the PR is ready. Changing the rolling_hash construction should be done in another pr, if we decided to do it.
Could you review it, please?

bh2smith

I've reviewed before, but now it looks so much better. Great work! 🧀

… Copy trait

josojo · 2019-07-03T16:19:06Z

I pushed the code for a potential solution for using arrays for standing orders.

Using arrays, which for types not implementing the Copy trait is a f... pain.
See the
https://stackoverflow.com/questions/28656387/initialize-a-large-fixed-size-array-with-non-copy-types

I could not implement the macro solution, as rust does not implement a clean way to share a macro over several crates
I could not use the Default trait, as it would be required to be implemented in std::default::Default.
I can not write the

impl Default for [StandingOrder; models::NUM_RESERVED_ACCOUNTS]

because only traits defined in the current crate can be implemented for arbitrary types.

Now, I came up with a solution, but it uses unsafe code. I think it would be cleaner and safer to just use vectors instead of unsafe code.

driver/src/db_interface.rs

driver/src/models/standing_order.rs

driver/src/db_interface.rs

driver/src/models/mod.rs

bh2smith

All looks really nice. Getting rustier and rustier

fleupold

Looks good to me. I'm just a bit confused by the non-rust code changes.

docker/driver/base/Dockerfile

.travis.yml

josojo · 2019-07-16T07:31:30Z

I could not find a solution for the clippy issue:
rust-lang/rust-clippy#3874

--cap-lints allow would not work, as we want warnings also to trigger a failure of test

--- locally I can just run:
- echo "cognitive-complexity-threshold = 30" > ~/.cargo/registry/src/github.com-1ecc6299db9ec823/mongodb-0.3.12/clippy.toml
and everything works. But this is of course not a good solution.
Also, because the file is created in the moment of running clippy, this does not work in Travis.

Does anyone have a suggestion?

.travis.yml

bh2smith · 2019-07-16T13:12:12Z

Hey @josojo - It appears there is a problem compiling a couple dependencies? Is this because of the rust version or clippy?

1.83s$ cargo clippy -- cap-lints
    Checking mongodb v0.3.12
    Checking tokio-uds v0.1.7
error: multiple input filenames provided (first two filenames are `/home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-uds-0.1.7/src/lib.rs` and `cap-lints`)
error: Could not compile `tokio-uds`.
warning: build failed, waiting for other jobs to finish...
error: multiple input filenames provided (first two filenames are `/home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/mongodb-0.3.12/src/lib.rs` and `cap-lints`)
error: Could not compile `mongodb`.
To learn more, run the command again with --verbose.
The command "cargo clippy -- cap-lints" exited with 101.

bh2smith · 2019-07-17T11:29:45Z

Just a question here:
Does pinning to a specific commit of a repo mean that we should keep an eye open for the next release which includes these changes?

josojo · 2019-07-17T13:41:32Z

Yes, totally.

get_standing_orders_of_slot will always return models::NUM_OF_ACCOUNT…

cf7e37f

… entries

josojo changed the title ~~Change get_standing_orders_of_slot to return always all standing_orders, including non-placed ones~~ [draft]Change get_standing_orders_of_slot to return always all standing_orders, including non-placed ones Jun 21, 2019

josojo requested review from fleupold and bh2smith June 21, 2019 09:28

josojo changed the title ~~[draft]Change get_standing_orders_of_slot to return always all standing_orders, including non-placed ones~~ Change get_standing_orders_of_slot to return always all standing_orders, including non-placed ones Jun 21, 2019

bh2smith reviewed Jun 21, 2019

View reviewed changes

driver/src/models/standing_order.rs Outdated Show resolved Hide resolved

josojo commented Jun 21, 2019

View reviewed changes

fleupold reviewed Jun 21, 2019

View reviewed changes

incorporated all inprovement suggestions, just refactoring

b0ed4e8

josojo requested review from fleupold and bh2smith June 24, 2019 06:03

bh2smith approved these changes Jun 24, 2019

View reviewed changes

fleupold reviewed Jun 24, 2019

View reviewed changes

driver/src/db_interface.rs Outdated Show resolved Hide resolved

less clonely solution from Felix + merge from master

4e3b417

josojo force-pushed the newSQLqueryWithZeroEntires branch from c2d6857 to 4e3b417 Compare June 27, 2019 09:26

bh2smith approved these changes Jul 3, 2019

View reviewed changes

using unsafe to initialize array, as Standingorder does not implement…

d2f8dd3

… Copy trait

clean up, sorry

c44bc0e

fleupold approved these changes Jul 4, 2019

View reviewed changes

driver/src/db_interface.rs Outdated Show resolved Hide resolved

driver/src/db_interface.rs Show resolved Hide resolved

driver/src/models/standing_order.rs Outdated Show resolved Hide resolved

bh2smith reviewed Jul 4, 2019

View reviewed changes

driver/src/db_interface.rs Outdated Show resolved Hide resolved

bh2smith reviewed Jul 4, 2019

View reviewed changes

driver/src/models/mod.rs Outdated Show resolved Hide resolved

bh2smith approved these changes Jul 4, 2019

View reviewed changes

josojo added 2 commits July 5, 2019 10:21

indentation

2bc45a6

uses array-macro to initialize the arrays

e7278c2

josojo added 5 commits July 11, 2019 19:58

test

d98e2f7

changing to newer rust version 1.36

c557130

test only

69b9ed9

another test to make rust run on travis server

54c6dd5

just testing travis

f4f8a2e

fleupold reviewed Jul 12, 2019

View reviewed changes

docker/driver/base/Dockerfile Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

josojo added 6 commits July 12, 2019 10:25

reverting all unnecessary changes

012052d

updating rust to 1.36 in docker images

4f47af1

making clippy work: see here: rust-lang/rust-clippy#3874

3b7c40d

stupid testing as I can not yet test it locally

2c83625

worst testing

31ef9db

clean up

476d5ce

josojo added 3 commits July 16, 2019 09:33

just another test

434b789

one more test

f1cd99a

final test

167fe52

bh2smith reviewed Jul 16, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

finally, Felix found the solution

4445f25

josojo merged commit 5e50642 into master Jul 17, 2019

fleupold deleted the newSQLqueryWithZeroEntires branch July 22, 2019 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change get_standing_orders_of_slot to return always all standing_orders, including non-placed ones #155

Change get_standing_orders_of_slot to return always all standing_orders, including non-placed ones #155

josojo commented Jun 21, 2019

josojo Jun 21, 2019

fleupold Jun 21, 2019

josojo Jun 24, 2019

fleupold left a comment

fleupold Jun 21, 2019

bh2smith left a comment

fleupold left a comment

bh2smith commented Jun 26, 2019

josojo commented Jun 27, 2019

fleupold commented Jul 1, 2019

josojo commented Jul 1, 2019

josojo commented Jul 3, 2019

bh2smith left a comment

josojo commented Jul 3, 2019 •

edited

Loading

bh2smith left a comment •

edited

Loading

fleupold left a comment

josojo commented Jul 16, 2019

bh2smith commented Jul 16, 2019

bh2smith commented Jul 17, 2019

josojo commented Jul 17, 2019

Change get_standing_orders_of_slot to return always all standing_orders, including non-placed ones #155

Change get_standing_orders_of_slot to return always all standing_orders, including non-placed ones #155

Conversation

josojo commented Jun 21, 2019

josojo Jun 21, 2019

Choose a reason for hiding this comment

fleupold Jun 21, 2019

Choose a reason for hiding this comment

josojo Jun 24, 2019

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

fleupold Jun 21, 2019

Choose a reason for hiding this comment

bh2smith left a comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

bh2smith commented Jun 26, 2019

josojo commented Jun 27, 2019

fleupold commented Jul 1, 2019

josojo commented Jul 1, 2019

josojo commented Jul 3, 2019

bh2smith left a comment

Choose a reason for hiding this comment

josojo commented Jul 3, 2019 • edited Loading

bh2smith left a comment • edited Loading

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

josojo commented Jul 16, 2019

bh2smith commented Jul 16, 2019

bh2smith commented Jul 17, 2019

josojo commented Jul 17, 2019

josojo commented Jul 3, 2019 •

edited

Loading

bh2smith left a comment •

edited

Loading