Add concurrent adding columns (ALTER TABLE ADD COLUMN) test to BaseConnectorTest #12483
Conversation
ebyhr
added
the
no-release-notes
ebyhr
force-pushed
the
ebi/test-concurrent-add-column
branch
2 times, most recently
from
20bd646 to
c4c227b
Compare
ebyhr
changed the title
|
CI hit #11634 |
| @Override | ||
| public void testAddColumnConcurrently() | ||
| { | ||
| // Difficult to determine whether the exception is concurrent issue or not from the error message |
There was a problem hiding this comment.
Let it render as a TODO in an IDE
| // Difficult to determine whether the exception is concurrent issue or not from the error message | |
| // TODO: Difficult to determine whether the exception is concurrent issue or not from the error message |
| @@ -255,6 +255,14 @@ protected String errorMessageForInsertIntoNotNullColumn(String columnName) | |||
| return format("NULL not allowed for column \"%s\"(?s).*", columnName.toUpperCase(ENGLISH)); | |||
| } | |||
|
|
|||
| @Test(enabled = false) | |||
There was a problem hiding this comment.
redundant over throw new SkipException in override
| @Test(enabled = false) |
| @Override | ||
| public void testAddColumnConcurrently() | ||
| { | ||
| // Default storage engine doesn't support adding new columns |
There was a problem hiding this comment.
| // Default storage engine doesn't support adding new columns | |
| // TODO: Default storage engine doesn't support adding new columns |
| protected void verifyConcurrentAddColumnFailurePermissible(Exception e) | ||
| { | ||
| assertThat(e) | ||
| .hasMessageContaining("Cannot update Iceberg table: supplied previous location does not match current location"); |
There was a problem hiding this comment.
This probably should be "Failed to commit Iceberg .."
but is fine for now.
cc @alexjo2144
| @Override | ||
| public void testAddColumnConcurrently() | ||
| { | ||
| throw new SkipException("TODO: Enable after supporting multi-document transaction https://www.mongodb.com/docs/manual/core/transactions/"); |
There was a problem hiding this comment.
remove @Test(enabled = false) (redundant)
and add a // TODO: ... comment so that an IDE can highlight this as a TODO
| .hasMessageContaining("Failed to perform metadata operation") | ||
| .getCause() | ||
| .hasMessageMatching( | ||
| "(?s).*SQLIntegrityConstraintViolationException.*" + | ||
| "|.*Unique index or primary key violation.*" + | ||
| "|.*Deadlock found when trying to get lock; try restarting transaction.*"); |
| .mapToObj(threadNumber -> executor.submit(() -> { | ||
| barrier.await(30, SECONDS); | ||
| try { | ||
| getQueryRunner().execute("ALTER TABLE " + tableName + " ADD COLUMN col" + threadNumber + " integer"); |
There was a problem hiding this comment.
Declare String columnName = "col" + threadNumber;
and make the Future a Future<Optional<String>> by retuning the columnName on success
| })) | ||
| .collect(toImmutableList()); | ||
|
|
||
| List<Integer> values = futures.stream() |
There was a problem hiding this comment.
List<Integer> values -> List<String> addedColumns
| if (values.isEmpty()) { | ||
| assertThat(query("DESCRIBE " + tableName)) | ||
| .projected(0) | ||
| .skippingTypesCheck() | ||
| .matches("VALUES ('col')"); | ||
| } | ||
| else { | ||
| assertThat(query("DESCRIBE " + tableName)) | ||
| .projected(0) | ||
| .skippingTypesCheck() | ||
| .matches(values.stream() | ||
| .map(value -> format("('col%s')", value)) | ||
| .collect(joining(",", "VALUES ('col'),", ""))); | ||
| } |
There was a problem hiding this comment.
You can handle both cases with single assert using Stream.concat:
.matches(Stream.concat(Stream.of("col"), addedColumns.stream())
.map(value -> format("'%s'", value))
.collect(joining(",", "VALUES ", "")));
| verifyConcurrentAddColumnFailurePermissible(trinoException); | ||
| } | ||
| catch (Throwable verifyFailure) { | ||
| if (trinoException != e && verifyFailure != e) { |
There was a problem hiding this comment.
i don't know why we have trinoException != e &&
i think it comes from earlier tests with similar pattern, but i don't remember why the first one (mine) has this
| .map(OptionalInt::getAsInt) | ||
| .collect(toImmutableList()); | ||
|
|
||
| if (values.isEmpty()) { |
There was a problem hiding this comment.
Should at least one of these always succeed?
There was a problem hiding this comment.
We can require at least one success, but then we may need to provide a connector a way to opt-out.
(i am fine not requiring this, and fine requiring this; and opt-out can be added later, if not needed now)
ebyhr
force-pushed
the
ebi/test-concurrent-add-column
branch
from
230f829 to
3a4168d
Compare
Description
Add concurrent adding columns test to BaseConnectorTest
Documentation
(x) No documentation is needed.
Release notes
(x) No release notes entries required.