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

implement PerfBackgroundJsonValidation feature-flag #21569

Merged
merged 13 commits into from
Jan 31, 2023

Conversation

colesnodgrass
Copy link
Member

@colesnodgrass colesnodgrass commented Jan 19, 2023

What

How

  • start plumbing in the FeatureFlagClient in all the necessary places to get feature-flag support within the RecordSchemaValidator

    • the existing FeatureFlags is still present in these files as I wanted to limit the scope changes to implement this single PerfBackgroundJsonValidation flag
  • this feature-flag doesn't do anything other than change a log message, once functionality has been verified, this will be updated to do real work

  • the feature-flag check is done in the following way:

    if (workspaceId != null) {
      if (ffClient.enabled(PerfBackgroundJsonValidation.INSTANCE, new Workspace(workspaceId))) {
        log.info("feature flag enabled for workspace {}", workspaceId);
      } else {
        log.info("feature flag disabled for workspace {}", workspaceId);
      }
    } else {
      log.info("workspace id is null");
    }

    I am unsure if a workspaceId will exist at this point in the code (the syncInput object supports a workspaceId but doesn't require one)

Lessons Learned

Micronaut property checking issues

Micronaut does not handle the following situation well:

if property P is defined and P is a non-empty string, then return bean A, else return bean B

Instead redefine the situation as:

if property P is defined and equals a well known value, then return bean A, else return B_

To summarize use @Requires(property=P, value=WELL_DEFINED) with @Requires(property=P, notEquals=WELL_DEFINED). Do not use @Requires(property=P) with @Requires(missingProperty=P) or @Requires(property=P, pattern=NON_EMPTY_REGEX) with @Requires(property=P, pattern=EMPTY_REGEX)

This also appears to be related to this Micronaut issue

Micronaut Kotlin Dependencies

I spent a lot of time with trying to get the Kotlin beans injectable into the java code. What should just work doesn't and it well documented on the Micronaut side as to what exactly is required (or at least I never found the documentation). After many attempts I and looking at the auto-generated kotlin Micronaut projects I landed on kapt being necessary in kotlin projects. Even this isn't entirely true.

If the Kotlin project is defined with the following gradle setup, the beans will not be visible to java projects (and possibly other projects as well):

plugins {
    kotlin("jvm") version "1.8.0"
}

dependencies {
    annotationProcessor(platform(libs.micronaut.bom))
    annotationProcessor(libs.bundles.micronaut.annotation.processor)
}

But they will be visible with the following setup:

plugins {
    kotlin("jvm") version "1.8.0"
    id("io.micronaut.minimal.application") version "3.7.0"
}

dependencies {
    annotationProcessor(platform(libs.micronaut.bom))
    annotationProcessor(libs.bundles.micronaut.annotation.processor)
}

micronaut {
  version("3.8.2")
}

and also visible with the following setup:

plugins {
    kotlin("jvm") version "1.8.0"
    kotlin("kapt") version "1.8.0"
}

dependencies {
    kapt(platform(libs.micronaut.bom))
    kapt(libs.bundles.micronaut.annotation.processor)
}

I went with the kapt solution.

It's worth noting that kapt is deprecated and is being replaced with kps in Micronaut 4 (this is a good thing).

@colesnodgrass colesnodgrass requested a review from a team as a code owner January 19, 2023 00:21
@octavia-squidington-iv octavia-squidington-iv added area/documentation Improvements or additions to documentation area/platform issues related to the platform area/worker Related to worker labels Jan 19, 2023
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 00:23 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 00:23 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2023

Airbyte Code Coverage

File Coverage [21.16%]
RecordSchemaValidator.java 95.08% 🍏
ContainerOrchestratorFactory.java 92.45% 🍏
ReplicationJobOrchestrator.java 11.67%
ReplicationActivityImpl.java 0.65%
TestConfigHelpers.java 0%
Total Project Coverage 24.51%

@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 00:52 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 00:52 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 00:56 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 00:56 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 00:58 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass requested a review from a team as a code owner January 19, 2023 04:48
@CLAassistant
Copy link

CLAassistant commented Jan 19, 2023

CLA assistant check
All committers have signed the CLA.

@octavia-squidington-iv octavia-squidington-iv removed the area/frontend Related to the Airbyte webapp label Jan 26, 2023
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 17:15 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 17:15 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 30, 2023 20:00 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 30, 2023 20:00 — with GitHub Actions Inactive
@@ -4,11 +4,12 @@ import org.jetbrains.kotlin.gradle.tasks.KotlinCompile
plugins {
`java-library`
kotlin("jvm") version "1.8.0"
kotlin("kapt") version "1.8.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a koitlin specific annotation processor?

@@ -43,6 +57,16 @@ public void validateSchema(final AirbyteRecordMessage message, final AirbyteStre
final JsonNode messageData = message.getData();
final JsonNode matchingSchema = streams.get(messageStream);

if (workspaceId != null) {
if (featureFlagClient.enabled(PerfBackgroundJsonValidation.INSTANCE, new Workspace(workspaceId))) {
log.info("feature flag enabled for workspace {}", workspaceId);
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the separate threading changes? Or does this PR only include the feature flag changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

also to confirm, we'll opt in workspaces via adding the workspace id in LauchDarkly?

Copy link
Member Author

Choose a reason for hiding this comment

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

This only includes the feature-flag changes. A quick, follow-up PR will add in the actual changes that will be contained within the feature-flag.

This allows me to vet that the client is working as expected and seeing updated from LaunchDarkly without having an impact on any users.

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

@colesnodgrass I think we are going to have a merge conflict. Do you mind letting me merge in my PR first before we merge in yours?

@davinchia davinchia temporarily deployed to more-secrets January 30, 2023 23:51 — with GitHub Actions Inactive
@davinchia davinchia temporarily deployed to more-secrets January 30, 2023 23:51 — with GitHub Actions Inactive
@davinchia davinchia temporarily deployed to more-secrets January 31, 2023 00:01 — with GitHub Actions Inactive
@davinchia davinchia temporarily deployed to more-secrets January 31, 2023 00:02 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants