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.

Sign up for GitHub

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

Jump to bottom

fix: generate random index name for change streams #32689

Merged
merged 15 commits into from
Oct 21, 2024
Merged

fix: generate random index name for change streams #32689

merged 15 commits into from
Oct 21, 2024

Conversation

thiagotnunes
Copy link
Contributor

Generates index names for change stream partition metadata table using a random UUID. This prevents issues if the job is being redeployed in an existing database.

@thiagotnunes
fix: generate random index name for change streams
a11e393 
Generates index names for change stream partition metadata table using a
random UUID. This prevents issues if the job is being redeployed in an
existing database.
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 8, 2024

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@thiagotnunes
Copy link
Contributor Author

retest this please

@thiagotnunes
fix: drops indexes from metadata table
6fcc3c2 
Finds and drops all indexes on the metadata table before attempting to
drop it.
@thiagotnunes
refactor: refactor name generator
ba4e3d1
@thiagotnunes
fix: always generates index names
3df479e 
If a table is given, we still need to generate the index names in case
the given table does not exist and needs to be created.
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 9, 2024

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damondouglas for label java.
R: @ahmedabu98 for label io.
R: @nielm for label spanner.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@nielm
Copy link
Contributor

nielm commented Oct 11, 2024 •
edited
Loading

Is this a fix for issue #32509? If so, can you add a "fixes: " in the description please?
@thiagotnunes

@nielm
Copy link
Contributor

nielm commented Oct 11, 2024

LGTM

@thiagotnunes
Copy link
Contributor Author

@nielm no, it is not a fix for #32509. AFAIU that issue happens, because the user cannot specify a table schema for the metadata table.

@nielm
Copy link
Contributor

nielm commented Oct 15, 2024

@ahmedabu98 for next reviewer in chain

@thiagotnunes
Copy link
Contributor Author

gentle ping @ahmedabu98 =)

scwhittle
scwhittle requested changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@scwhittle scwhittle left a comment

Choose a reason for hiding this comment

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

Given there were issues with the index, should we add a pipeline option controlling whether the index is used or not? Or perhaps there would be too much code complication by having two paths, what do you think?

...ain/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/dao/PartitionMetadataAdminDao.java
@@ -129,7 +123,7 @@ public void createPartitionMetadataTable() {
// Literals need be added around literals to preserve casing.
ddl.add(
"CREATE TABLE \""
+ tableName
+ names.getTableName()
Copy link
Contributor

Choose a reason for hiding this comment

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

does this method fail if the tables/indexes already exist?

IIUC the issue correctly this is failing currently because the index has the same name across pipelines and the creation is failing. Do we also have to worry about a single pipeline failing if it created the tables but Dataflow retries the work? It seems we might want ot use create if it doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Since the names have a random component, I assumed the job would fail and we would re-generate them.

Updated with IF NOT EXISTS to circumvent this problem.

...n/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/dao/PartitionMetadataTableNames.java Show resolved Hide resolved
...va/org/apache/beam/sdk/io/gcp/spanner/changestreams/dao/PartitionMetadataTableNamesTest.java Outdated Show resolved Hide resolved
...va/org/apache/beam/sdk/io/gcp/spanner/changestreams/dao/PartitionMetadataTableNamesTest.java Outdated Show resolved Hide resolved
@scwhittle scwhittle merged commit ac87d7b into apache:master Oct 21, 2024
18 checks passed
@thiagotnunes thiagotnunes deleted the fix-metadata-index branch October 22, 2024 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scwhittle scwhittle scwhittle approved these changes

@dedocibula dedocibula dedocibula approved these changes

Assignees
No one assigned
Labels
gcp io java Next Action: Reviewers spanner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thiagotnunes @nielm @dedocibula @scwhittle