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

Disable cron for SlackHQ Upstream Sync action #4

Closed
wants to merge 7 commits into from

Conversation

timvaillancourt
Copy link
Member

Description

This PR stops the SlackHQ Upstream Sync action from running on a cron schedule so it doesn't fight a POC being tested at https://github.com/slackhq/vitess-repo-syncer

Copy link

@vmogilev vmogilev left a comment

Choose a reason for hiding this comment

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

A few Qs just so I understand how this works:

  1. is the other repo now responsible for the sync of this repo?
  2. if ^^^ is true, do we still need .github/workflows/slackhq_upstream_sync.yml, or this is just a quick test?

@timvaillancourt
Copy link
Member Author

timvaillancourt commented Aug 10, 2022

@vmogilev I'm testing two approaches at the moment:

  1. The GitHub Action on slackhq/vitess itself. The drawback here is we must add a commit to the main branch for every merge/syncing and any changes to our slackhq_upstream_sync.yml. Example:
    Screen Shot 2022-08-10 at 21 48 26

  2. Running the GitHub Action from slack/vitess-repo-syncer with git push --force. This approach causes no new commits to main and sounds like the better approach, although it requires an extra private repo. Another benefit is we can edit the README.md to explain how it works, troubleshooting, etc

I'd like to keep both approaches while we decide which is best. For now disabling the cron schedule will prevent them from fighting each other

cc @tanjinx

@tanjinx
Copy link

tanjinx commented Aug 15, 2022

here is we must add a commit to the main branch for every merge/syncing and any changes
@timvaillancourt I am no longer seeing those commits, do we confirm if this is true?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ timvaillancourt
✅ tanjinx
❌ actions-user
You have signed the CLA already but the status is still pending? Let us recheck it.

@timvaillancourt
Copy link
Member Author

Closing as the repo sync moved to a new repo

@timvaillancourt timvaillancourt deleted the tvaillancourt-disable-sync-action-cron branch August 18, 2022 17:45
brirams pushed a commit that referenced this pull request Sep 22, 2022
* 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 <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

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

Signed-off-by: Max Englander <[email protected]>

* revert fmt changes

Signed-off-by: Max Englander <[email protected]>

* implement pr review suggestion

Signed-off-by: Max Englander <[email protected]>

Signed-off-by: Max Englander <[email protected]>
@tanjinx tanjinx added the v12 label Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants