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.

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

Jump to bottom

VtGateV3 Lookup testcases #5

Merged
merged 6 commits into from
Oct 15, 2019
Merged

VtGateV3 Lookup testcases #5

merged 6 commits into from
Oct 15, 2019

Conversation

ajeetj
Copy link

@ajeetj ajeetj commented Oct 11, 2019 •
edited by deepthi
Loading

Migrated VtGateV3 lookup testcases from Python to Go.
Summary of tests:

  • Test non-unique vindexes
  • Test select by id
  • Test select by lookup
  • Test IN clause using non-unique vindex
  • Test unowned functional vindex
  • Test updates to secondary vindexes
  • Test updating both vindexes
  • Test updating only one vindex
  • Test owned lookup unique index
  • Test unowned lookup unique index

Detail about each case is present in below sheet:
https://docs.google.com/spreadsheets/d/1fMv0ehVMeQNVvjl3HAzZ7LuHzRaIZrjwQrENF0O3iE8/edit#gid=0

@ajeetj
added secondary lookup test cases
8efb82a 
Signed-off-by: Ajeet jain <ajeet@planetscale.com>
@ajeetj
added secondary lookup test cases
a47aabf 
Signed-off-by: Ajeet jain <ajeet@planetscale.com>
@ajeetj
added unowned and owned lookup tests
1fb7784 
Signed-off-by: Ajeet jain <ajeet@planetscale.com>
@ajeetj
Remove commented code
a3ec645 
Signed-off-by: Ajeet jain <ajeet@planetscale.com>
@ajeetj ajeetj requested a review from deepthi October 11, 2019 08:26
@ajeetj ajeetj requested a review from sougou as a code owner October 11, 2019 08:26
@ajeetj ajeetj changed the title VtGateV3 Lookup testcases [Do Not Merge] VtGateV3 Lookup testcases Oct 11, 2019
@ajeetj
Copy link
Author

ajeetj commented Oct 11, 2019

@deepthi I still need to understand a few cases which I have commented out [We can discuss in our next syncup]. Besides that this PR is good to go.

deepthi
deepthi reviewed Oct 12, 2019
View reviewed changes
Copy link
Collaborator

@deepthi deepthi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work! Looks good on a first pass.
I will look at the details in the spreadsheet and do a more thorough review once I get access to it.

@ajeetj
updated scatter update test
5facf10 
Signed-off-by: Ajeet jain <ajeet@planetscale.com>
@ajeetj ajeetj changed the title [Do Not Merge] VtGateV3 Lookup testcases VtGateV3 Lookup testcases Oct 14, 2019
@ajeetj
Copy link
Author

ajeetj commented Oct 14, 2019

Thanks @deepthi
I also updated one test and converted it into error handling.

There is 1 question/verification pending from my side where a duplicate insert into a unique column should fail, but it is not. I have added a TODO and will verify again when we have a new local cluster ready [Arindam is currently working on that].

This PR is good to merge now.

deepthi
deepthi reviewed Oct 15, 2019
View reviewed changes
Copy link
Collaborator

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Looks good except for a few nit-picks.

go/vt/vtgate/endtoend/lookup_test.go

// insert multiple values
exec(t, conn, "begin")
exec(t, conn, "insert into t3(user_id, lastname, address) values(1,'snow','castle_black'), (2,'stark','winterfell')")
Copy link
Collaborator

Choose a reason for hiding this comment

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

haha

go/vt/vtgate/endtoend/lookup_test.go
exec(t, conn, "insert into t3(user_id, lastname, address) values(1,'snow','castle_black'), (2,'stark','winterfell')")
exec(t, conn, "commit")
qr := exec(t, conn, "select * from t3")
if got, want := fmt.Sprintf("%v", qr.Rows), "[[INT64(1) VARCHAR(\"snow\") VARCHAR(\"castle_black\")] [INT64(2) VARCHAR(\"stark\") VARCHAR(\"winterfell\")]]"; got != want {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the order of rows expected to be stable?

Copy link
Author

Choose a reason for hiding this comment

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

Added order by clause

go/vt/vtgate/endtoend/lookup_test.go Outdated
t.Errorf("select:\n%v want\n%v", got, want)
}

//update both videxes
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: vindexes

go/vt/vtgate/endtoend/lookup_test.go Outdated
}
}

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use // style doc comments instead of multi-line comments.

go/vt/vtgate/endtoend/lookup_test.go Outdated
}
}

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above.

deepthi
deepthi approved these changes Oct 15, 2019
View reviewed changes
Copy link
Collaborator

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Can merge after addressing feedback.

@ajeetj
review comments addressed
cb8e4e0 
Signed-off-by: Ajeet jain <ajeet@planetscale.com>
@ajeetj
Copy link
Author

ajeetj commented Oct 15, 2019

Can merge after addressing feedback.

Addressed review comments, merging now.

@ajeetj ajeetj merged commit c01fe37 into tal_vtgate Oct 15, 2019
@ajeetj ajeetj deleted the tal_vtgate_lookup branch October 15, 2019 04:16
harshit-gangal pushed a commit that referenced this pull request Sep 7, 2022
@maxenglander
decouple olap tx timeout from oltp tx timeout (vitessio#10946)
3e3365e 
* decouple olap tx timeout from oltp tx timeout

Since workload=olap bypasses the query timeouts
(--queryserver-config-query-timeout) and also row limits, the natural
assumption is that it also bypasses the transaction timeout.

This is not the case, e.g. for a tablet where the
--queryserver-config-transaction-timeout is 10.

This commit:

 * Adds new CLI flag and YAML field to independently configure TX
   timeouts for OLAP workloads (--queryserver-config-olap-transaction-timeout).
 * Decouples TX kill interval from OLTP TX timeout via new CLI flag and
   YAML field (--queryserver-config-transaction-killer-interval).

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #1

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #2 consolidate timeout logic in sc

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: remove unused tx killer flag

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: update 15_0_0_summary.md

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: fix race cond

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #3 -txProps.timeout, +sc.expiryTime

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #4 -atomic.Value for expiryTime

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: fix race cond (without atomic.Value)

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #5 -unused funcs, fix comments, set ticks interval once

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #5 +txkill tests

Signed-off-by: Max Englander <max@planetscale.com>

* revert fmt changes

Signed-off-by: Max Englander <max@planetscale.com>

* implement pr review suggestion

Signed-off-by: Max Englander <max@planetscale.com>

Signed-off-by: Max Englander <max@planetscale.com>
notfelineit pushed a commit that referenced this pull request Sep 21, 2022
@maxenglander
decouple olap tx timeout from oltp tx timeout (vitessio#1040)
c5a99ec 
* decouple olap tx timeout from oltp tx timeout

Since workload=olap bypasses the query timeouts
(--queryserver-config-query-timeout) and also row limits, the natural
assumption is that it also bypasses the transaction timeout.

This is not the case, e.g. for a tablet where the
--queryserver-config-transaction-timeout is 10.

This commit:

 * Adds new CLI flag and YAML field to independently configure TX
   timeouts for OLAP workloads (--queryserver-config-olap-transaction-timeout).
 * Decouples TX kill interval from OLTP TX timeout via new CLI flag and
   YAML field (--queryserver-config-transaction-killer-interval).

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #1

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #2 consolidate timeout logic in sc

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: remove unused tx killer flag

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: update 15_0_0_summary.md

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: fix race cond

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #3 -txProps.timeout, +sc.expiryTime

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #4 -atomic.Value for expiryTime

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: fix race cond (without atomic.Value)

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #5 -unused funcs, fix comments, set ticks interval once

Signed-off-by: Max Englander <max@planetscale.com>

* decouple ol{a,t}p tx timeouts: pr comments #5 +txkill tests

Signed-off-by: Max Englander <max@planetscale.com>

* fix flags

Signed-off-by: Max Englander <max@planetscale.com>

Signed-off-by: Max Englander <max@planetscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepthi deepthi deepthi approved these changes

@sougou sougou Awaiting requested review from sougou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ajeetj @deepthi