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

Bmoric/update connection list with breaking #18125

Merged
merged 54 commits into from
Oct 24, 2022
Merged
Changes from 1 commit
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
abaea95
add schemaChange
alovew Oct 10, 2022
f428057
merge conflict
alovew Oct 11, 2022
5a3df67
frontend tests
alovew Oct 11, 2022
6b8e1f0
tests
alovew Oct 12, 2022
5ac2cde
l
alovew Oct 12, 2022
024b3d3
fix source catalog id
alovew Oct 13, 2022
4fcc5a6
test
alovew Oct 13, 2022
b8eeec2
formatting
alovew Oct 13, 2022
ce5b574
move schema change to build backend web connection
alovew Oct 13, 2022
b14b322
check if actor catalog id is different
alovew Oct 13, 2022
9593ebc
fix
alovew Oct 13, 2022
558022c
tests and fixes
alovew Oct 14, 2022
17a9681
remove extra var
alovew Oct 14, 2022
e03cdbb
remove logging
alovew Oct 14, 2022
2686be6
continue to pass back new catalog id
alovew Oct 14, 2022
62ca0f7
api updates
alovew Oct 17, 2022
b78fd6e
fix mockdata
alovew Oct 17, 2022
7941c76
tests
alovew Oct 17, 2022
c42af6b
add schemaChange
alovew Oct 10, 2022
c444399
merge conflict
alovew Oct 11, 2022
2359d9d
frontend tests
alovew Oct 11, 2022
50770ae
tests
alovew Oct 12, 2022
5bc3e3e
l
alovew Oct 12, 2022
6d13a42
fix source catalog id
alovew Oct 13, 2022
20f9e30
test
alovew Oct 13, 2022
21eb1d0
formatting
alovew Oct 13, 2022
32613c0
move schema change to build backend web connection
alovew Oct 13, 2022
5320f73
check if actor catalog id is different
alovew Oct 13, 2022
826cfb0
fix
alovew Oct 13, 2022
1270de2
tests and fixes
alovew Oct 14, 2022
f10e413
remove extra var
alovew Oct 14, 2022
1c9a008
remove logging
alovew Oct 14, 2022
5781ce0
continue to pass back new catalog id
alovew Oct 14, 2022
5203c9f
api updates
alovew Oct 17, 2022
f81285c
fix mockdata
alovew Oct 17, 2022
6c4d620
tests
alovew Oct 17, 2022
9e1db24
tests
alovew Oct 17, 2022
e16a35d
optional of nullable
alovew Oct 17, 2022
1e9cc8a
Tmp
benmoriceau Oct 17, 2022
f38b682
For diff
benmoriceau Oct 18, 2022
8f28aa5
Add test
benmoriceau Oct 18, 2022
acf9ec1
More test
benmoriceau Oct 18, 2022
fc085cf
Fix test and add some
benmoriceau Oct 19, 2022
95a6dc0
Merge branch 'anne/connection-get-endpoint' of github.com:airbytehq/a…
benmoriceau Oct 19, 2022
cdf1065
Fix merge and test
benmoriceau Oct 19, 2022
d1683d5
Fix PMD
benmoriceau Oct 19, 2022
6662bb7
Merge branch 'master' into anne/connection-get-endpoint
benmoriceau Oct 19, 2022
f93f67d
Merge branch 'anne/connection-get-endpoint' into bmoric/update-connec…
benmoriceau Oct 19, 2022
a2b00ca
Merge branch 'master' of github.com:airbytehq/airbyte into bmoric/upd…
benmoriceau Oct 21, 2022
53937da
Fix test
benmoriceau Oct 21, 2022
8c8cd4b
Rm dead code
benmoriceau Oct 21, 2022
0a0c500
Fix pmd
benmoriceau Oct 21, 2022
f4959c8
Address PR comments
benmoriceau Oct 24, 2022
68d6825
RM unused column
benmoriceau Oct 24, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address PR comments
  • Loading branch information
benmoriceau committed Oct 24, 2022
commit f4959c84d192985affefcb76fa3e40c600da24ac
Original file line number Diff line number Diff line change
@@ -71,7 +71,6 @@
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import lombok.Data;
import org.apache.commons.lang3.ArrayUtils;
import org.jooq.Condition;
import org.jooq.DSLContext;
@@ -976,45 +975,20 @@ public Optional<ActorCatalogFetchEvent> getMostRecentActorCatalogFetchEventForSo
return records.stream().findFirst().map(DbConverter::buildActorCatalogFetchEvent);
}

@Data
public static class ActorCatalogFetchEventWithCreationDate {

private final ActorCatalogFetchEvent actorCatalogFetchEvent;
private final OffsetDateTime createdAt;

}

public Map<UUID, ActorCatalogFetchEventWithCreationDate> getMostRecentActorCatalogFetchEventForSources(final List<UUID> sourceIds)
public Map<UUID, ActorCatalogFetchEvent> getMostRecentActorCatalogFetchEventForSources(final List<UUID> sourceIds)
Copy link
Contributor

@cgardens cgardens Nov 20, 2022

Choose a reason for hiding this comment

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

@benmoriceau what is this method supposed to do? the argument that is passed in is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgardens. It is doing a similar operation than getMostRecentActorCatalogFetchEventForSource but for a list of sourceIds instead of a single source Id. The use of the input got lost during PR updates

Fixed in #19668

throws IOException {

return database.query(ctx -> ctx.select(ACTOR_CATALOG_FETCH_EVENT.asterisk())
.from(ACTOR_CATALOG_FETCH_EVENT)
.where(ACTOR_CATALOG_FETCH_EVENT.ACTOR_ID.in(sourceIds))
.fetch()
.stream()
.map(record -> new ActorCatalogFetchEventWithCreationDate(
DbConverter.buildActorCatalogFetchEvent(record),
record.get(ACTOR_CATALOG_FETCH_EVENT.CREATED_AT)))
.collect(
() -> new HashMap<>(),
this::insertInAccumulatorIfNeeded,
(left, right) -> {
right.forEach((actorId, value) -> {
insertInAccumulatorIfNeeded(left, value);
});
}));
}

private void insertInAccumulatorIfNeeded(final Map<UUID, ActorCatalogFetchEventWithCreationDate> acc,
final ActorCatalogFetchEventWithCreationDate value) {
if (acc.containsKey(value.getActorCatalogFetchEvent().getActorId())) {
final ActorCatalogFetchEventWithCreationDate currentNewest = acc.get(value.actorCatalogFetchEvent.getActorId());
if (currentNewest.getCreatedAt().isBefore(value.getCreatedAt())) {
acc.put(value.getActorCatalogFetchEvent().getActorId(), value);
}
} else {
acc.put(value.actorCatalogFetchEvent.getActorId(), value);
}
return database.query(ctx -> ctx.fetch(
"""
select actor_catalog_id, actor_id from
(select id, actor_catalog_id, actor_id, config_hash, actor_version, created_at, rank() over (partition by actor_id order by created_at desc) as creation_order_rank, modified_at
Copy link
Contributor

Choose a reason for hiding this comment

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

kind of nitpicky, but do we need to select all these fields or just actor_id, actor_catalog_id, & creation_order_rank?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes most of the column are not needed. I have remove them. Thanks for catching that.

from public.actor_catalog_fetch_event
) table_with_rank
where creation_order_rank = 1;
"""))
.stream().map(DbConverter::buildActorCatalogFetchEvent)
.collect(Collectors.toMap(record -> record.getActorId(),
record -> record));
}

/**
Original file line number Diff line number Diff line change
@@ -30,7 +30,6 @@
import io.airbyte.config.StandardSync;
import io.airbyte.config.StandardSyncOperation;
import io.airbyte.config.StandardWorkspace;
import io.airbyte.config.persistence.ConfigRepository.ActorCatalogFetchEventWithCreationDate;
import io.airbyte.config.persistence.ConfigRepository.DestinationAndDefinition;
import io.airbyte.config.persistence.ConfigRepository.SourceAndDefinition;
import io.airbyte.config.persistence.split_secrets.JsonSecretsProcessor;
@@ -547,21 +546,21 @@ void testGetMostRecentActorCatalogFetchEventForSources() throws SQLException, IO
}

database.transaction(ctx -> {
MockData.actorCatalogFetchEventsForAggregationTest().forEach(actorCatalogFetchEventWithCreationDate -> insertCatalogFetchEvent(
MockData.actorCatalogFetchEventsForAggregationTest().forEach(actorCatalogFetchEvent -> insertCatalogFetchEvent(
ctx,
actorCatalogFetchEventWithCreationDate.getActorCatalogFetchEvent().getActorId(),
actorCatalogFetchEventWithCreationDate.getActorCatalogFetchEvent().getActorCatalogId(),
actorCatalogFetchEventWithCreationDate.getCreatedAt()));
actorCatalogFetchEvent.getActorCatalogFetchEvent().getActorId(),
actorCatalogFetchEvent.getActorCatalogFetchEvent().getActorCatalogId(),
actorCatalogFetchEvent.getCreatedAt()));

return null;
});

final Map<UUID, ActorCatalogFetchEventWithCreationDate> result =
final Map<UUID, ActorCatalogFetchEvent> result =
configRepository.getMostRecentActorCatalogFetchEventForSources(List.of(MockData.SOURCE_ID_1,
MockData.SOURCE_ID_2));

assertEquals(MockData.ACTOR_CATALOG_ID_1, result.get(MockData.SOURCE_ID_1).getActorCatalogFetchEvent().getActorCatalogId());
assertEquals(MockData.ACTOR_CATALOG_ID_3, result.get(MockData.SOURCE_ID_2).getActorCatalogFetchEvent().getActorCatalogId());
assertEquals(MockData.ACTOR_CATALOG_ID_1, result.get(MockData.SOURCE_ID_1).getActorCatalogId());
assertEquals(MockData.ACTOR_CATALOG_ID_3, result.get(MockData.SOURCE_ID_2).getActorCatalogId());
}

private void insertCatalogFetchEvent(final DSLContext ctx, final UUID sourceId, final UUID catalogId, final OffsetDateTime creationDate) {
Original file line number Diff line number Diff line change
@@ -39,7 +39,6 @@
import io.airbyte.config.WebhookConfig;
import io.airbyte.config.WebhookOperationConfigs;
import io.airbyte.config.WorkspaceServiceAccount;
import io.airbyte.config.persistence.ConfigRepository.ActorCatalogFetchEventWithCreationDate;
import io.airbyte.protocol.models.AirbyteCatalog;
import io.airbyte.protocol.models.AuthSpecification;
import io.airbyte.protocol.models.AuthSpecification.AuthType;
@@ -59,6 +58,7 @@
import java.util.TreeMap;
import java.util.UUID;
import java.util.stream.Collectors;
import lombok.Data;

public class MockData {

@@ -646,6 +646,14 @@ public static List<ActorCatalogFetchEvent> actorCatalogFetchEventsSameSource() {
return Arrays.asList(actorCatalogFetchEvent1, actorCatalogFetchEvent2);
}

@Data
public static class ActorCatalogFetchEventWithCreationDate {

private final ActorCatalogFetchEvent actorCatalogFetchEvent;
private final OffsetDateTime createdAt;

}

public static List<ActorCatalogFetchEventWithCreationDate> actorCatalogFetchEventsForAggregationTest() {
final OffsetDateTime now = OffsetDateTime.now();
final OffsetDateTime yesterday = OffsetDateTime.now().minusDays(1l);
Original file line number Diff line number Diff line change
@@ -48,7 +48,6 @@
import io.airbyte.config.StandardSync;
import io.airbyte.config.persistence.ConfigNotFoundException;
import io.airbyte.config.persistence.ConfigRepository;
import io.airbyte.config.persistence.ConfigRepository.ActorCatalogFetchEventWithCreationDate;
import io.airbyte.protocol.models.ConfiguredAirbyteCatalog;
import io.airbyte.server.converters.ApiPojoConverters;
import io.airbyte.server.handlers.helpers.CatalogConverter;
@@ -115,7 +114,7 @@ public WebBackendConnectionReadList webBackendListConnectionsForWorkspace(final
final Map<UUID, JobRead> runningJobByConnectionId =
getRunningJobByConnectionId(standardSyncs.stream().map(StandardSync::getConnectionId).toList());

final Map<UUID, ActorCatalogFetchEventWithCreationDate> newestFetchEventsByActorId =
final Map<UUID, ActorCatalogFetchEvent> newestFetchEventsByActorId =
configRepository.getMostRecentActorCatalogFetchEventForSources(new ArrayList<>());
Copy link
Contributor

Choose a reason for hiding this comment

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

@benmoriceau why are we adding more db queries into this handler? it's really specifically not supposed to be making direct database calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgardens It was not very clear that the repository shouldn't have been used here. Especially since it was recently in in the same endpoint and other endpoints to get data from the DB. The ticket related to this PR #17526 is only needing the described functionality in the webBackend endpoint only so it made sense to add that direct call in the WebBackendHandler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@benmoriceau benmoriceau Nov 22, 2022

Choose a reason for hiding this comment

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

This comment is easy to miss when the same field is being used within the same function here. When you are looking at the function to update, it is not explicit that this is a deprecated field. Something like this would make it very explicit that this is deprecated. It will show the getter are an error and would have prevent any use of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks good. let's do that. i also put up a PR that adds more explanation in the javadocs #19719


final List<WebBackendConnectionListItem> connectionItems = Lists.newArrayList();
@@ -197,7 +196,7 @@ private WebBackendConnectionListItem buildWebBackendConnectionListItem(
final Map<UUID, DestinationRead> destinationReadById,
final Map<UUID, JobRead> latestJobByConnectionId,
final Map<UUID, JobRead> runningJobByConnectionId,
final Optional<ActorCatalogFetchEventWithCreationDate> latestFetchEvent)
final Optional<ActorCatalogFetchEvent> latestFetchEvent)
throws JsonValidationException, ConfigNotFoundException, IOException {

final SourceRead source = sourceReadById.get(standardSync.getSourceId());
@@ -207,11 +206,7 @@ private WebBackendConnectionListItem buildWebBackendConnectionListItem(
final ConnectionRead connectionRead = connectionsHandler.getConnection(standardSync.getConnectionId());
final Optional<UUID> currentCatalogId = connectionRead == null ? Optional.empty() : Optional.ofNullable(connectionRead.getSourceCatalogId());
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible for connectionread to be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing tells me that the field is not nullable so I prefered to handle the possibility.


final Optional<ActorCatalogFetchEvent> mostRecentFetchEvent =
latestFetchEvent
.map(actorCatalogFetchEventWithCreationDate -> actorCatalogFetchEventWithCreationDate.getActorCatalogFetchEvent());

final SchemaChange schemaChange = getSchemaChange(connectionRead, currentCatalogId, mostRecentFetchEvent);
final SchemaChange schemaChange = getSchemaChange(connectionRead, currentCatalogId, latestFetchEvent);

final WebBackendConnectionListItem listItem = new WebBackendConnectionListItem()
.connectionId(standardSync.getConnectionId())