update acceptance tests to use new connection scheduling format #16167
Conversation
mfsiega-airbyte
requested review from
a team and
pmossman
and removed request for
a team
mfsiega-airbyte
force-pushed
the
msiega/cronstrings
branch
from
bdeaa67
to
8726a04
Compare
github-actions
bot
added
area/platform
mfsiega-airbyte
force-pushed
the
msiega/cronstrings
branch
from
8726a04
to
13e43fa
Compare
There was a problem hiding this comment.
@mfsiega-airbyte this looks good to me! I added a few questions/recommendations but I'll mark this as approved and let you decide how many of the suggestions you want to adopt or not.
| testHarness.createConnection(TEST_CONNECTION, sourceId, destinationId, List.of(operationId), catalog, ConnectionScheduleType.BASIC, | ||
| BASIC_SCHEDULE_DATA).getConnectionId(); | ||
|
|
||
| for (int i = 0; i < 10; i++) { |
There was a problem hiding this comment.
This is basically just to impose a limit on how many times the test checks for the job to succeed, right? Might be worth a comment, or maybe worth adding some explicit logging in the case where we tried 10 times and the job was still waiting so that it's very clear in the case where this test fails
| final AirbyteCatalog catalog = testHarness.discoverSourceSchema(sourceId); | ||
|
|
||
| final ConnectionScheduleData connectionScheduleData = new ConnectionScheduleData().cron( | ||
| new ConnectionScheduleDataCron().cronExpression("* */2 * * * ?").cronTimeZone("UTC")); |
There was a problem hiding this comment.
I'd add a comment explaining in human language what this cron expression means. I think this one is 'every 2 minutes', right?
This also makes me think it might be worth adding another test case, for a cron expression that we know shouldn't run any time soon, particularly since before we introduced cron schedules, a new scheduled connection always ran immediately
There was a problem hiding this comment.
Added a comment to clarify what the cron means.
I do like the idea of another test - in the interest of getting something merged I'll go ahead for now, but I'll add a note to the overall ticket to revisit before closing.
| @@ -35,7 +35,8 @@ public static ImmutableMap<String, Object> generateSyncMetadata(final StandardSy | |||
| metadata.put("connection_id", standardSync.getConnectionId()); | |||
|
|
|||
| final String frequencyString; | |||
| if (standardSync.getManual()) { | |||
| // TODO(https://github.com/airbytehq/airbyte/issues/2170): handle cron strings properly. | |||
There was a problem hiding this comment.
So this basically relies on us "dual-writing" to the old schedule column, and the linked issue is for the clean-up task that will ultimately deprecate the old schedule column, right?
Would it make sense to update the code here right now to prefer the new column if set, and only fall back to getSchedule if the new column wasn't available?
We could even add some logging that tells us whenever this code had to use the old schedule. That way, when we eventually get around to removing the old schedule, we'll have logs that can help improve our confidence that the column is indeed safe to remove
There was a problem hiding this comment.
Resolved in a separate PR.
github-actions
bot
removed
area/scheduler
area/platform
…ytehq#16167) * update acceptance tests to use new scheduling format now that frontend has moved over * readability improvements in sync schedule tests * fix pmd issues in new acceptance test
…ytehq#16167) * update acceptance tests to use new scheduling format now that frontend has moved over * readability improvements in sync schedule tests * fix pmd issues in new acceptance test
What
The frontend has moved to the new connection schedule format, so we now want our acceptance tests to also use this path.
Also adds an e2e test for cron scheduling.