search

crate / crate-qa

CrateDB Quality Assurance
4 stars 1 forks source link

Add test for ingesting data while doing a rolling upgrade #273

Closed mkleen closed 1 year ago

mkleen commented 1 year ago

Summary of the changes / Why this is an improvement

Resolves https://github.com/crate/crate-qa/issues/271

Checklist

mkleen commented 1 year ago

retest this please

mkleen commented 1 year ago

retest this please

mkleen commented 1 year ago

Not perfect, but runs stable and consistent now.

mfussenegger commented 1 year ago

Thinking a bit more about this I'm actually not sure what this covers additionally to the existing RollingUpgradeTest ? That does cover having a mixed version cluster and doing inserts.

The concurrent insert/select aspect is as far as I can tell not that interesting, as it's covered plenty in integration tests in crate/crate, and the mixed version aspect afaik shouldn't have any impact on it.

I thought what we wanted to extend on is the data types used and also use index off.

mkleen commented 1 year ago

Thinking a bit more about this I'm actually not sure what this covers additionally to the existing RollingUpgradeTest ? That does cover having a mixed version cluster and doing inserts.

The concurrent insert/select aspect is as far as I can tell not that interesting, as it's covered plenty in integration tests in crate/crate, and the mixed version aspect afaik shouldn't have any impact on it.

I thought what we wanted to extend on is the data types used and also use index off.

I was following https://github.com/crate/crate-qa/issues/271 and I am ok to skip it if it's not useful and redundant.

mfussenegger commented 1 year ago

Thinking a bit more about this I'm actually not sure what this covers additionally to the existing RollingUpgradeTest ? That does cover having a mixed version cluster and doing inserts. The concurrent insert/select aspect is as far as I can tell not that interesting, as it's covered plenty in integration tests in crate/crate, and the mixed version aspect afaik shouldn't have any impact on it. I thought what we wanted to extend on is the data types used and also use index off.

I was following #271 and I am ok to skip it if it's not useful and redundant.

I see. I'm not sure what large inserts would additionally cover. Maybe let's put this on hold until Basti is back and we can clarify this. I suspect triggering the recovery logic could be an aspect, but I'm not sure if that's the best way to go about it.

We also recently extended the data type tests in CrateDB to ensure indexers+mapper results in the same fields: https://github.com/crate/crate/blob/9ebfd11a6cf70becf617fe1762bd4a513285ad2d/server/src/testFixtures/java/io/crate/types/DataTypeTestCase.java#L125-L140, so that kind of error scenario would now be covered (at least once we've a test case for each type, which isn't the case yet)

mkleen commented 1 year ago

I see. I'm not sure what large inserts would additionally cover. Maybe let's put this on hold until Basti is back and we can clarify this. I suspect triggering the recovery logic could be an aspect, but I'm not sure if that's the best way to go about it.

Sounds good.