-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Data Validation] Add Spanner failure injection test + Base class #4124
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
base: main
Are you sure you want to change the base?
Changes from all commits
1a7b908
b8ec286
36a0b46
614eeef
1778460
e75b5cd
9cba4df
894aa41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |||||
| import java.time.Duration; | ||||||
| import java.time.Instant; | ||||||
| import java.time.format.DateTimeParseException; | ||||||
| import java.util.concurrent.atomic.AtomicLong; | ||||||
| import org.slf4j.Logger; | ||||||
| import org.slf4j.LoggerFactory; | ||||||
|
|
||||||
|
|
@@ -37,12 +38,12 @@ public class InitialLimitedDurationErrorInjectionPolicy | |||||
| LoggerFactory.getLogger(InitialLimitedDurationErrorInjectionPolicy.class); | ||||||
| private static final long serialVersionUID = 1L; | ||||||
|
|
||||||
| private Instant startTime; | ||||||
| private static volatile Instant startTime = null; | ||||||
| private static final AtomicLong callCount = new AtomicLong(0); | ||||||
| private final Duration injectionDuration; | ||||||
| private final String effectiveDurationParameter; | ||||||
| private String errorCodeToBeInjected; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||
| private Clock clock; | ||||||
| private long callCount; | ||||||
|
|
||||||
| private static final String DEFAULT_DURATION = "PT10M"; | ||||||
| private static final String DURATION_FIELD_IN_OBJECT = "duration"; | ||||||
|
|
@@ -124,22 +125,20 @@ public InitialLimitedDurationErrorInjectionPolicy(JsonNode inputParameter, Clock | |||||
| */ | ||||||
| @Override | ||||||
| public boolean shouldInjectionError() { | ||||||
| if (this.startTime == null) { | ||||||
| synchronized (this) { | ||||||
| if (this.startTime == null) { | ||||||
| this.startTime = Instant.now(clock); | ||||||
| if (startTime == null) { | ||||||
| synchronized (InitialLimitedDurationErrorInjectionPolicy.class) { | ||||||
| if (startTime == null) { | ||||||
| startTime = Instant.now(clock); | ||||||
|
Comment on lines
+129
to
+131
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are synchronising on the class now instead of the object of the class? Do I understand that right? What was the issue happening earlier?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What was happening earlier: I fixed that by making the state static so it's shared across the worker. But once the state is static, locking on this (the instance) is unsafe because threads using different instances would acquire different locks, leading to race conditions. Synchronizing on the Class object ensures all instances share the exact same lock to initialize the global timer safely. |
||||||
| LOG.info( | ||||||
| "First call detected. Errors will be injected for {} starting from {}.", | ||||||
| this.injectionDuration, | ||||||
| this.startTime); | ||||||
| startTime); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| synchronized (this) { | ||||||
| ++callCount; | ||||||
| } | ||||||
| long currentCallCount = callCount.incrementAndGet(); | ||||||
|
|
||||||
| if (callCount < INITIAL_ALLOWED_CALLS_COUNT) { | ||||||
| if (currentCallCount < INITIAL_ALLOWED_CALLS_COUNT) { | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -186,6 +185,11 @@ void setClockForTesting(Clock clock) { | |||||
| this.clock = clock; | ||||||
| } | ||||||
|
|
||||||
| public static void resetForTesting() { | ||||||
| startTime = null; | ||||||
| callCount.set(0); | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public String toString() { | ||||||
| return "InitialLimitedDurationErrorInjectionPolicy{" | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| /* | ||
| * Copyright (C) 2026 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not | ||
| * use this file except in compliance with the License. You may obtain a copy of | ||
| * the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| */ | ||
| package com.google.cloud.teleport.v2.templates; | ||
|
|
||
| import com.google.cloud.teleport.v2.spanner.migrations.transformation.CustomTransformation; | ||
| import com.google.common.io.Resources; | ||
| import java.io.IOException; | ||
| import java.util.Map; | ||
| import org.apache.beam.it.common.PipelineLauncher.LaunchInfo; | ||
| import org.apache.beam.it.common.utils.PipelineUtils; | ||
| import org.apache.beam.it.gcp.dataflow.FlexTemplateDataflowJobResourceManager; | ||
| import org.apache.beam.it.gcp.spanner.SpannerResourceManager; | ||
|
|
||
| /** | ||
| * Base class for gcs-spanner-dv failure injection integration tests. | ||
| * | ||
| * <p><strong>Why is this a separate class from {@link GCSSpannerDVITBase}?</strong> | ||
| * | ||
| * <p>While {@code GCSSpannerDVITBase} is runner-agnostic and relies on the generic {@code | ||
| * PipelineLauncher} (allowing tests to run locally via DirectRunner), failure injection testing | ||
| * explicitly requires building a custom Docker image with the {@code failureInjectionTest} Maven | ||
| * profile. | ||
| * | ||
| * <p>Therefore, tests extending this class are strictly coupled to Dataflow Flex Templates and | ||
| * bypass the generic launcher in favor of {@link FlexTemplateDataflowJobResourceManager}. | ||
| */ | ||
| public abstract class GCSSpannerDVFTBase extends GCSSpannerDVITBase { | ||
|
|
||
| /** | ||
| * Launches the Dataflow job with failure injection testing capabilities using | ||
| * FlexTemplateDataflowJobResourceManager. | ||
| */ | ||
| protected LaunchInfo launchFTDataflowJob( | ||
| String testId, | ||
| String projectId, | ||
| SpannerResourceManager spannerResourceManager, | ||
| String bigQueryDataset, | ||
| String gcsInputDirectory, | ||
| String sessionFileResourceName, | ||
| String schemaOverridesFileResourceName, | ||
| String tableOverrides, | ||
| String columnOverrides, | ||
| CustomTransformation customTransformation, | ||
| String failureInjectionParameter, | ||
| Map<String, String> jobParameters) | ||
| throws IOException { | ||
|
|
||
| FlexTemplateDataflowJobResourceManager.Builder flexTemplateBuilder = | ||
| FlexTemplateDataflowJobResourceManager.builder(testId) | ||
| .withTemplateName("GCS_Spanner_Data_Validator") | ||
| .withTemplateModulePath("v2/gcs-spanner-dv") | ||
| .withAdditionalMavenProfile("failureInjectionTest") | ||
| .addEnvironmentVariable( | ||
| "additionalExperiments", java.util.Collections.singletonList("disable_runner_v2")); | ||
|
|
||
| if (failureInjectionParameter != null && !failureInjectionParameter.isEmpty()) { | ||
| flexTemplateBuilder.addParameter("failureInjectionParameter", failureInjectionParameter); | ||
| } | ||
|
|
||
| flexTemplateBuilder.addParameter("projectId", projectId); | ||
| flexTemplateBuilder.addParameter("instanceId", spannerResourceManager.getInstanceId()); | ||
| flexTemplateBuilder.addParameter("databaseId", spannerResourceManager.getDatabaseId()); | ||
| flexTemplateBuilder.addParameter("bigQueryDataset", bigQueryDataset); | ||
| flexTemplateBuilder.addParameter("gcsInputDirectory", gcsInputDirectory); | ||
|
|
||
| if (sessionFileResourceName != null) { | ||
| gcsClient.uploadArtifact( | ||
| "session.json", Resources.getResource(sessionFileResourceName).getPath()); | ||
| flexTemplateBuilder.addParameter("sessionFilePath", getGcsPath("session.json")); | ||
| } | ||
|
|
||
| if (schemaOverridesFileResourceName != null) { | ||
| gcsClient.uploadArtifact( | ||
| "schema_overrides.json", | ||
| Resources.getResource(schemaOverridesFileResourceName).getPath()); | ||
| flexTemplateBuilder.addParameter( | ||
| "schemaOverridesFilePath", getGcsPath("schema_overrides.json")); | ||
| } | ||
|
|
||
| if (tableOverrides != null) { | ||
| flexTemplateBuilder.addParameter("tableOverrides", tableOverrides); | ||
| } | ||
|
|
||
| if (columnOverrides != null) { | ||
| flexTemplateBuilder.addParameter("columnOverrides", columnOverrides); | ||
| } | ||
|
|
||
| if (customTransformation != null) { | ||
| flexTemplateBuilder.addParameter( | ||
| "transformationJarPath", getGcsPath(customTransformation.jarPath())); | ||
| flexTemplateBuilder.addParameter("transformationClassName", customTransformation.classPath()); | ||
| if (customTransformation.customParameters() != null) { | ||
| flexTemplateBuilder.addParameter( | ||
| "transformationCustomParameters", customTransformation.customParameters()); | ||
| } | ||
| } | ||
|
|
||
| String runId = PipelineUtils.createJobName(testId); | ||
| flexTemplateBuilder.addParameter("runId", runId); | ||
| flexTemplateBuilder.addParameter("workerMachineType", "n2-standard-4"); | ||
|
|
||
| if (jobParameters != null) { | ||
| jobParameters.forEach(flexTemplateBuilder::addParameter); | ||
| } | ||
|
|
||
| return flexTemplateBuilder.build().launchJob(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was
AtomicLongneeded?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we made the state static, all the threads on a Dataflow worker are now hitting the exact same counter. So if we were to use synchronized block, every thread would have to acquire the lock and wait. AtomicLong is better as
incrementAndGet()allows threads to update the counter without blocking each other.