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

Test dynamic column names in resultsets with less partitions created #346

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jeeminso
Copy link
Contributor

@jeeminso jeeminso commented Apr 1, 2025

Summary of the changes / Why this is an improvement

Improves #344.

Checklist

  • Link to issue this PR refers to (if applicable): Fixes #???

@jeeminso jeeminso force-pushed the jeeminso/nonetype-error branch 4 times, most recently from 7e2318e to ffe5ee3 Compare April 1, 2025 20:08
@jeeminso jeeminso force-pushed the jeeminso/nonetype-error branch from ffe5ee3 to aa5bd13 Compare April 1, 2025 20:20
@jeeminso jeeminso changed the title debug Test dynamic column names in resultsets with less partitions created Apr 1, 2025
@jeeminso
Copy link
Contributor Author

jeeminso commented Apr 1, 2025

Hi @matriv , I think my previous approach created more partitions/shards than it needed to and caused time outs / weird state of the cluster leading to such exceptions. Could you take a look at this?

Also do you think it is worth to investigate how the null rows got inserted?

  File "/var/lib/jenkins/workspace/CrateDB/qa/crate_qa/tests/bwc/test_upgrade.py", line 326, in assert_data_persistence
    for name in row[0].keys():
                ^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'keys'

@jeeminso jeeminso marked this pull request as ready for review April 1, 2025 22:10
@jeeminso jeeminso requested a review from matriv April 1, 2025 22:10
@matriv
Copy link
Contributor

matriv commented Apr 2, 2025

Sorry I don't get your approach now, why do you need a new table, but keep a dynamic object on the partitioned table? Also why did you restrict the inserts to only 2 versions?

@jeeminso
Copy link
Contributor Author

jeeminso commented Apr 2, 2025

Sorry I don't get your approach now, why do you need a new table, but keep a dynamic object on the partitioned table? Also why did you restrict the inserts to only 2 versions?

Those are results of the reversion, it would be clearer to check the last commit of the two.

@matriv
Copy link
Contributor

matriv commented Apr 2, 2025

Thank you @jeeminso, I was confused before.

@matriv
Copy link
Contributor

matriv commented Apr 2, 2025

retest this please

@jeeminso
Copy link
Contributor Author

jeeminso commented Apr 3, 2025

The exception due to noneType is gone with the approved changes but still timing out, even removing most of the test added originally did not help. I am guessing it is some kind of env issue.

@matriv
Copy link
Contributor

matriv commented Apr 3, 2025

retest this please

3 similar comments
@jeeminso
Copy link
Contributor Author

jeeminso commented Apr 3, 2025

retest this please

@jeeminso
Copy link
Contributor Author

jeeminso commented Apr 3, 2025

retest this please

@jeeminso
Copy link
Contributor Author

jeeminso commented Apr 4, 2025

retest this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants