You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/10/10 13:42:20 UTC
[GitHub] [cassandra] dchenbecker opened a new pull request, #1904: Enable dtest-offheap in CircleCI
dchenbecker opened a new pull request, #1904:
URL: https://github.com/apache/cassandra/pull/1904
The dtest-offheap test was only added to the Jenkins configuration, so
this commit adds it to the CircleCI build for parity. Because
dtest-offheap in Jenkins is configured to skip resource intensive
tests, the dtest-offheap suite is added to all three sets (LOW,
MEDIUM, HIGH).
Patch by Derek Chen-Becker; reviewed by Ekaterina Dimitrova for CASSANDRA-17950
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on pull request #1904: Enable dtest-offheap in CircleCI
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on PR #1904:
URL: https://github.com/apache/cassandra/pull/1904#issuecomment-1283287493
Thank you for your work. There are a few things here which we need to work out:
1) Adding new jobs requires recreating the patches. (as per the instructions in the readme). Even if the patch applies, it might be moving things to different lines. If you try now to run `.circleci/generate.sh -m` you get:
```
Generating new config.yml file with middle resources from config-2_1.yml
patching file .circleci/config-2_1.yml
Hunk #1 succeeded at 111 (offset 6 lines).
Hunk #2 succeeded at 126 (offset 6 lines).
Hunk #3 succeeded at 168 (offset 6 lines).
Hunk #4 succeeded at 931 (offset 34 lines).
Hunk #5 succeeded at 945 with fuzz 1 (offset 34 lines).
Hunk #6 succeeded at 960 with fuzz 1 (offset 34 lines).
Hunk #7 succeeded at 974 (offset 34 lines).
Hunk #8 succeeded at 989 (offset 34 lines).
Hunk #9 succeeded at 1003 (offset 34 lines).
Hunk #10 succeeded at 1018 (offset 34 lines).
Hunk #11 succeeded at 1036 (offset 34 lines).
Hunk #12 succeeded at 1051 (offset 34 lines).
Hunk #13 succeeded at 1069 (offset 34 lines).
Hunk #14 succeeded at 1084 (offset 34 lines).
Hunk #15 succeeded at 1102 (offset 34 lines).
Hunk #16 succeeded at 1117 (offset 34 lines).
```
I just tried the following to regenerate the patches on your branch:
# apply old patches (with hunk messages)
patch -o config-2_1.yml.MIDRES config-2_1.yml config-2_1.yml.mid_res.patch
patch -o config-2_1.yml.HIGHRES config-2_1.yml config-2_1.yml.high_res.patch
# generate new patches
diff -u config-2_1.yml config-2_1.yml.HIGHRES > config-2_1.yml.high_res.patch
diff -u config-2_1.yml config-2_1.yml.MIDRES > config-2_1.yml.mid_res.patch
# verify that the new patches generate the same files and cleanup
./generate.sh -a"
This actually created some diff which was missing and it shows me that the update was not accurately done. (The parameter from the commit which is still not removed popped up now)
All in all the main goal is to ensure that when we apply the patches they create MIDRES and HIGHRES with all resources assigned to the old jobs as they were and adds a new job with the resources we want.
2) How were the midres resources chosen? Is there a CI run? I guess you just used the same resources as the regular dtests jobs which we have already? I would probably start from there too, but we need also to test. Do you have a paid account to run them or do you need help?
3) This is trunk, I thought we aim to align all branches? In this case those tests run in Jenkins on 3.11+
Normally we start creating patches on the lowest C* version we target branch and propagate up to trunk. You might want to check the Development section on our web site.
4) Agreed with @michaelsembwever, the second commit deserves its own ticket and commit
3) Andres committed today a patch that adds repeated jobs pre-commit, detection of changed/added new tests. You will need to rebase and figure out how your patch is affected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dchenbecker commented on pull request #1904: Enable dtest-offheap in CircleCI
Posted by GitBox <gi...@apache.org>.
dchenbecker commented on PR #1904:
URL: https://github.com/apache/cassandra/pull/1904#issuecomment-1280857027
I can separate it out. This is part of work to parameterize the CircleCI
config instead of using patch files to modify parameters. The former is
well-defined for CircleCI yaml while the patch approach is clunky and
error-prone. Is that worth a CEP or just a discussion on the mailing list?
Thanks,
Derek
On Mon, Oct 17, 2022, 3:41 AM mck ***@***.***> wrote:
> It's unclear to me why 00096ff
> <https://github.com/apache/cassandra/commit/00096ff0a644dd0fd0c00e688587ff077f4c260d>
> is included in this PR …?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/cassandra/pull/1904#issuecomment-1280581773>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAGCGWU7C4QU5CDSEYRDHTWDUNOHANCNFSM6AAAAAARBLSDQQ>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dchenbecker commented on pull request #1904: Enable dtest-offheap in CircleCI
Posted by GitBox <gi...@apache.org>.
dchenbecker commented on PR #1904:
URL: https://github.com/apache/cassandra/pull/1904#issuecomment-1281017489
Sure thing, I'll open a ticket and separate PR.
Cheers,
Derek
On Mon, Oct 17, 2022, 8:00 AM mck ***@***.***> wrote:
> I can separate it out. This is part of work to parameterize the CircleCI
> config instead of using patch files to modify parameters. The former is
> well-defined for CircleCI yaml while the patch approach is clunky and
> error-prone. Is that worth a CEP or just a discussion on the mailing list?
>
> Just a separate ticket and PR. (I don't disagree with it, just that we
> can't sneak it in under this PR and ticket.)
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/cassandra/pull/1904#issuecomment-1280910511>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAGCGVJIPCOC4WHNAGVWHLWDVLYJANCNFSM6AAAAAARBLSDQQ>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on pull request #1904: Enable dtest-offheap in CircleCI
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on PR #1904:
URL: https://github.com/apache/cassandra/pull/1904#issuecomment-1283293585
PS I didn't check what you run exactly in the jobs for now as I assumed it was mostly copy-paste. I will double-check this tomorrow, it's already late
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dchenbecker commented on pull request #1904: Enable dtest-offheap in CircleCI
Posted by GitBox <gi...@apache.org>.
dchenbecker commented on PR #1904:
URL: https://github.com/apache/cassandra/pull/1904#issuecomment-1284288101
To be clear, this PR was just to confirm I'm on the right path. Apologies if I made it sound like this was intended as a final patch. Having never worked with CircleCI before I just wasn't sure I was making the right change. Answers to your points follow:
1. I can certainly generate the patches if the base change looks correct. I do think we should consider abandoning the patch approach and switching to parameterized CircleCI config instead. The circleci CLI tool (most recent version) can take a second yaml file with the parameters and apply them to generate the LOW/MID/HIGHRES files instead, which is much less error prone and much simpler to reason about. The first commit (point #4) is an example of how that's done, so is the best approach there to open a new ticket for that work?
2. I don't have a paid account. When you say "midres resources" I'm not sure what you mean. I copied the job configuration for the offheap test run directly from the existing offheap test run in Jenkins. The num-tokens parameter is what every other test in the harness is using
3. I can certainly start with 3.11. Again, this was just a "am I doing this right" PR for review, not a final review. In regards to that, though, a quick perusal seems to indicate that the different branches are also diverged, but it's not clear what's meaningful. Everything but trunk has some extra comments and different names. This doesn't seem scalable, and I wonder if we should start with how we can simplify the build system in general before piling onto an already complex amalgam
4. In line with the previous comment, given how difficult it's been to figure out things on one branch, and then looking at the diffs across branches, I'm thinking maybe I should start with simplifying the build via parameterization (and other changes) before adding more tests
5. Let me figure out items 3 and 4 before I do any more work in the direction of adding tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on pull request #1904: Enable dtest-offheap in CircleCI
Posted by GitBox <gi...@apache.org>.
ekaterinadimitrova2 commented on PR #1904:
URL: https://github.com/apache/cassandra/pull/1904#issuecomment-1286372093
1) You closed the PR and the commit you mentioned is not linked anymore so I cannot confirm your approach there.
New ticket for improving CircleCI SGTM. I also shared since yesterday David's work for your consideration.
2) `I don't have a paid account.` I will run the tests when you are done then. We will figure it out.
` When you say "midres resources" I'm not sure what you mean.` - I refer to the MIDRES file which points to enough resources to run all suites with a paid account for a reasonable time. HIGHRES is if you want to bump on max the resources and break the bank. Not really significantly faster compared to MIDRES
3) "extra comments and different names" - I do not see extra comment on something as a big problem. What different names you refer to?
4) As we spoke in Slack, we need to first add the tests short term to unblock the project to be able to release based on CircleCI until we fix all the infra problems, etc in Jenkins.
Then we can open improvement ticket and gradually improve things according to the mailing list consensus, etc.
Honestly, there is in the readme explanation and CircleCI has a good documentation, but the issue is there are many moving parts and we need to be careful while updating things. I can see how easily we can mess up something with this approach as in time our test suites and JDKs grew significantly. Now, I think not doing the patches, etc might have been actually harder for you.
I also suggest you make a table with all suites you plan to add and suggestion for initial resources which can be approximate based on similar suites (resource_class and parallelism). Example - I think you did right by trying to assign the resources we use already for Python Dtests also to the off-heap DTests.
Do you still want me to look into the actual job content in detail (what you have in this PR) or you will be working on something else? Let me know
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on pull request #1904: Enable dtest-offheap in CircleCI
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on PR #1904:
URL: https://github.com/apache/cassandra/pull/1904#issuecomment-1280910511
> I can separate it out. This is part of work to parameterize the CircleCI config instead of using patch files to modify parameters. The former is well-defined for CircleCI yaml while the patch approach is clunky and error-prone. Is that worth a CEP or just a discussion on the mailing list?
Just a separate ticket and PR. (I don't disagree with it, just that we can't sneak it in under this PR and ticket.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on pull request #1904: Enable dtest-offheap in CircleCI
Posted by GitBox <gi...@apache.org>.
michaelsembwever commented on PR #1904:
URL: https://github.com/apache/cassandra/pull/1904#issuecomment-1280581773
It's unclear to me why 00096ff0a644dd0fd0c00e688587ff077f4c260d is included in this PR …?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dchenbecker closed pull request #1904: Enable dtest-offheap in CircleCI
Posted by GitBox <gi...@apache.org>.
dchenbecker closed pull request #1904: Enable dtest-offheap in CircleCI
URL: https://github.com/apache/cassandra/pull/1904
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org