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

Adds a new string cloumn to configs for cron #12416

Merged
merged 11 commits into from
May 18, 2022

Conversation

supertopher
Copy link
Contributor

Adds a new string cloumn to configs for cron
closes #11418
feedback very welcome nits included

What

Adds the column using generators

Unsure if we expect testing for these. Peter's PR certainly has tests but I thought I would ask the room here

@supertopher supertopher temporarily deployed to more-secrets April 27, 2022 20:09 Inactive
@supertopher supertopher temporarily deployed to more-secrets April 27, 2022 20:09 Inactive
@supertopher supertopher marked this pull request as ready for review April 27, 2022 20:17
@supertopher supertopher temporarily deployed to more-secrets April 27, 2022 20:17 Inactive
@supertopher supertopher temporarily deployed to more-secrets April 27, 2022 20:17 Inactive
Copy link
Contributor

@malikdiarra malikdiarra left a comment

Choose a reason for hiding this comment

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

Regarding tests, I usually find that test on migration are not the most useful. Usually some tests are added later on data access pattern on the table. Those tests would also implicitly test that the migration works correctly.

@CLAassistant
Copy link

CLAassistant commented May 5, 2022

CLA assistant check
All committers have signed the CLA.

@supertopher supertopher force-pushed the toph-CRON-new-schedule-column branch from 7743156 to 7cb6428 Compare May 10, 2022 13:16
@supertopher supertopher temporarily deployed to more-secrets May 10, 2022 13:18 Inactive
@supertopher supertopher temporarily deployed to more-secrets May 10, 2022 13:18 Inactive
@supertopher supertopher force-pushed the toph-CRON-new-schedule-column branch from 7cb6428 to 6e747c9 Compare May 17, 2022 20:52
@supertopher supertopher temporarily deployed to more-secrets May 17, 2022 20:54 Inactive
@supertopher supertopher temporarily deployed to more-secrets May 17, 2022 20:54 Inactive
@supertopher supertopher temporarily deployed to more-secrets May 17, 2022 22:02 Inactive
@supertopher supertopher temporarily deployed to more-secrets May 17, 2022 22:02 Inactive
@supertopher
Copy link
Contributor Author

@davinchia I'm getting a pretty weird error on my branch when I try to run
SUB_BUILD=PLATFORM ./gradlew :airbyte-db:jooq:compileJava

I'll look into the gradle now, but I'm getting rate limit errors on my docker hub account trying to compile the jooq.

this error loops a few times a second (explaining the rate limit part anyway)

        at java.lang.Thread.run(Thread.java:833) [?:?]
2022-05-17 21:59:51 WARN o.t.i.RemoteDockerImage(resolve):104 - Retrying pull for image: testcontainers/ryuk:0.3.3 (118s remaining)
2022-05-17 21:59:51 ERROR c.g.d.a.a.ResultCallbackTemplate(onError):52 - Error during callback
com.github.dockerjava.api.exception.InternalServerErrorException: Status 500: {"message":"Head \"https://registry-1.docker.io/v2/testcontainers/ryuk/manifests/0.3.3\": toomanyrequests: too many failed login attempts for username or IP address"}

        at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.execute(DefaultInvocationBuilder.java:247) ~[testcontainers-1.17.1.jar:?]
        at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.lambda$executeAndStream$1(DefaultInvocationBuilder.java:269) ~[testcontainers-1.17.1.jar:?]
        at java.lang.Thread.run(Thread.java:833) [?:?]
2022-05-17 21:59:51 WARN o.t.i.RemoteDockerImage(resolve):104 - Retrying pull for image: testcontainers/ryuk:0.3.3 (118s remaining)
2022-05-17 21:59:52 ERROR c.g.d.a.a.ResultCallbackTemplate(onError):52 - Error during callback
com.github.dockerjava.api.exception.InternalServerErrorException: Status 500: {"message":"Head \"https://registry-1.docker.io/v2/testcontainers/ryuk/manifests/0.3.3\": toomanyrequests: too many failed login attempts for username or IP address"}

        at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.execute(DefaultInvocationBuilder.java:247) ~[testcontainers-1.17.1.jar:?]
        at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.lambda$executeAndStream$1(DefaultInvocationBuilder.java:269) ~[testcontainers-1.17.1.jar:?]
        at java.lang.Thread.run(Thread.java:833) [?:?]
2022-05-17 21:59:52 WARN o.t.i.RemoteDockerImage(resolve):104 - Retrying pull for image: testcontainers/ryuk:0.3.3 (117s remaining)
2022-05-17 21:59:52 ERROR c.g.d.a.a.ResultCallbackTemplate(onError):52 - Error during callback
com.github.dockerjava.api.exception.InternalServerErrorException: Status 500: {"message":"Head \"https://registry-1.docker.io/v2/testcontainers/ryuk/manifests/0.3.3\": toomanyrequests: too many failed login attempts for username or IP address"}

        at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.execute(DefaultInvocationBuilder.java:247) ~[testcontainers-1.17.1.jar:?]
        at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.lambda$executeAndStream$1(DefaultInvocationBuilder.java:269) ~[testcontainers-1.17.1.jar:?]
        at java.lang.Thread.run(Thread.java:833) [?:?]
2022-05-17 21:59:52 WARN o.t.i.RemoteDockerImage(resolve):104 - Retrying pull for image: testcontainers/ryuk:0.3.3 (117s remaining)

as to the code itself, I expect it to compile correctly but I cannot verify that running the jooq command because of docker

@davinchia
Copy link
Contributor

@supertopher I think that might just be your local. This is what I get when I try to run this:

Caused by: org.postgresql.util.PSQLException: ERROR: type "schedule_type" does not exist
  Position: 58
        at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2675) ~[postgresql-42.3.4.jar:42.3.4]
        at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2365) ~[postgresql-42.3.4.jar:42.3.4]
        at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:355) ~[postgresql-42.3.4.jar:42.3.4]
        at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:490) ~[postgresql-42.3.4.jar:42.3.4]
        at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:408) ~[postgresql-42.3.4.jar:42.3.4]
        at org.postgresql.jdbc.PgPreparedStatement.executeWithFlags(PgPreparedStatement.java:167) ~[postgresql-42.3.4.jar:42.3.4]
        at org.postgresql.jdbc.PgPreparedStatement.execute(PgPreparedStatement.java:156) ~[postgresql-42.3.4.jar:42.3.4]
        at com.zaxxer.hikari.pool.ProxyPreparedStatement.execute(ProxyPreparedStatement.java:44) ~[HikariCP-5.0.1.jar:?]
        at com.zaxxer.hikari.pool.HikariProxyPreparedStatement.execute(HikariProxyPreparedStatement.java) ~[HikariCP-5.0.1.jar:?]
        at org.jooq.tools.jdbc.DefaultPreparedStatement.execute(DefaultPreparedStatement.java:209) ~[jooq-3.13.4.jar:?]
        at org.jooq.impl.AbstractQuery.execute(AbstractQuery.java:453) ~[jooq-3.13.4.jar:?]
        at org.jooq.impl.AbstractQuery.execute(AbstractQuery.java:371) ~[jooq-3.13.4.jar:?]
        at io.airbyte.db.instance.configs.migrations.V0_36_3_001__AddScheduleTypeToConfigsTable.addPublicColumn(V0_36_3_001__AddScheduleTypeToConfigsTable.java:70) ~[io.airbyte.airbyte-db-lib-0.38.4-alpha.jar:?]
        at io.airbyte.db.instance.configs.migrations.V0_36_3_001__AddScheduleTypeToConfigsTable.migrate(V0_36_3_001__AddScheduleTypeToConfigsTable.java:62) ~[io.airbyte.airbyte-db-lib-0.38.4-alpha.jar:?]
        at org.flywaydb.core.internal.resolver.java.JavaMigrationExecutor.executeOnce(JavaMigrationExecutor.java:61) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.resolver.java.JavaMigrationExecutor.lambda$execute$0(JavaMigrationExecutor.java:54) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.database.DefaultExecutionStrategy.execute(DefaultExecutionStrategy.java:27) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.resolver.java.JavaMigrationExecutor.execute(JavaMigrationExecutor.java:53) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.command.DbMigrate.doMigrateGroup(DbMigrate.java:370) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.command.DbMigrate.lambda$applyMigrations$1(DbMigrate.java:271) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.jdbc.TransactionalExecutionTemplate.execute(TransactionalExecutionTemplate.java:66) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.command.DbMigrate.applyMigrations(DbMigrate.java:270) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.command.DbMigrate.migrateGroup(DbMigrate.java:243) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.command.DbMigrate.lambda$migrateAll$0(DbMigrate.java:141) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.database.postgresql.PostgreSQLAdvisoryLockTemplate.execute(PostgreSQLAdvisoryLockTemplate.java:70) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.database.postgresql.PostgreSQLConnection.lock(PostgreSQLConnection.java:99) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.schemahistory.JdbcTableSchemaHistory.lock(JdbcTableSchemaHistory.java:141) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.command.DbMigrate.migrateAll(DbMigrate.java:141) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.internal.command.DbMigrate.migrate(DbMigrate.java:101) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.Flyway$1.execute(Flyway.java:217) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.Flyway$1.execute(Flyway.java:168) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.Flyway.execute(Flyway.java:584) ~[flyway-core-7.14.0.jar:?]
        at org.flywaydb.core.Flyway.migrate(Flyway.java:168) ~[flyway-core-7.14.0.jar:?]
        at io.airbyte.db.instance.FlywayDatabaseMigrator.migrate(FlywayDatabaseMigrator.java:36) ~[io.airbyte.airbyte-db-lib-0.38.4-alpha.jar:?]
        at io.airbyte.db.instance.FlywayMigrationDatabase.createInternalConnection(FlywayMigrationDatabase.java:92) ~[io.airbyte.airbyte-db-lib-0.38.4-alpha.jar:?]
        at io.airbyte.db.instance.FlywayMigrationDatabase.getInternalConnection(FlywayMigrationDatabase.java:66) ~[io.airbyte.airbyte-db-lib-0.38.4-alpha.jar:?]
        ... 8 more

@davinchia davinchia temporarily deployed to more-secrets May 18, 2022 11:22 Inactive
@davinchia davinchia temporarily deployed to more-secrets May 18, 2022 11:22 Inactive
@davinchia davinchia temporarily deployed to more-secrets May 18, 2022 11:37 Inactive
@davinchia davinchia temporarily deployed to more-secrets May 18, 2022 11:37 Inactive
@davinchia davinchia temporarily deployed to more-secrets May 18, 2022 12:30 Inactive
@davinchia davinchia temporarily deployed to more-secrets May 18, 2022 12:30 Inactive
@supertopher supertopher temporarily deployed to more-secrets May 18, 2022 13:24 Inactive
@supertopher supertopher temporarily deployed to more-secrets May 18, 2022 13:24 Inactive
@supertopher supertopher dismissed malikdiarra’s stale review May 18, 2022 13:27

outdated by newer commits and work with Davin

@supertopher supertopher temporarily deployed to more-secrets May 18, 2022 13:37 Inactive
@supertopher supertopher temporarily deployed to more-secrets May 18, 2022 13:37 Inactive
@supertopher supertopher merged commit 3aca043 into master May 18, 2022
@supertopher supertopher deleted the toph-CRON-new-schedule-column branch May 18, 2022 14:03
suhomud pushed a commit that referenced this pull request May 23, 2022
* Adds a new string cloumn to configs for cron

closes #11418
i'm new to this task in Java please be brutal

* Adds airbyte header

* WIP

* Rebase a week of commits

* WIP for davin

* deps update

* Reorganize code for better readability. Also add a schema.

* Update tests.

* Correct bad test.

* Adds note for testing version change

* formatting change

Co-authored-by: Davin Chia <davinchia@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cron String: Add new schedule_type column.
5 participants