SED-4617 implement-detailed-inline-node-details-mode-for-step-list#132
SED-4617 implement-detailed-inline-node-details-mode-for-step-list#132david-stephan wants to merge 7 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an INCLUDES filter for collections, enabling array containment checks in both MongoDB and PostgreSQL, and updates the OQL grammar to support this new operator. It also implements an asynchronous migration task framework with status tracking in MigrationManager and optimizes index creation for list fields in PostgreSQL. Additionally, the PR includes improvements to AbstractIdentifiableObject's custom field handling. Feedback highlights concerns regarding the lifecycle management of the newly created executor, potential memory side effects in AbstractIdentifiableObject, and the need for a more robust JSON serialization approach in the PostgreSQL filter factory.
jeromecomte
left a comment
jeromecomte
left a comment
There was a problem hiding this comment.
Posting intermediate review status
| .filter(t -> t instanceof AsyncMigrationTask) | ||
| .map(t -> (AsyncMigrationTask) t) | ||
| .filter(t -> t.asOfVersion.compareTo(to) <= 0) | ||
| .sorted((a, b) -> a.asOfVersion.compareTo(b.asOfVersion)) |
There was a problem hiding this comment.
We never used it until now, but sync migration tasks support downgrades. For async migration tasks it doesn't seem to be the case. It would be fine for me but in this case I would completely skip the async path for downgrades in order to avoid unexpected behaviours
There was a problem hiding this comment.
The downdgrade is indeed not supported in async (mentioned in javadoc) and is already not part of the async path. "runDowngradeScript" is only called from the method "migrate" which explicitly excludes "AsyncMigrationTask".
Note that all async tasks are executed if not yet marked as completed (and not just the ones introduced between the versions from - to) because we cannot be sure that they completed after the last upgrade. I mean I upgrade to 29.1, it starts the async tasks registered for 29.1, I upgrade to 29.2, it will run the async tasks for 29.1 if they were not yet completed.
Please give me more details if I misunderstood your point "I would completely skip the async path for downgrades"
There was a problem hiding this comment.
I meant the case where from > to. If I don't miss anything, currently the method migrateAsync will also be called in this case and would try to do something. Shouldn't we execute it only if from < to?
There was a problem hiding this comment.
as discussed the filter already excludes task that are registered for a version > to (.filter(t -> t.asOfVersion.compareTo(to) <= 0))
| } | ||
|
|
||
| private String formatIncludesValue(Object expectedValue) { | ||
| Object valueToSerialize = (expectedValue instanceof ObjectId) |
There was a problem hiding this comment.
In theory we should use the same ObjectMapper as the one registered for Jackson. This would handle ObjectId and the rest properly
There was a problem hiding this comment.
We are already using the same Object mapper (PostgreSQLCollectionJacksonMapperProvider -> DefaultJacksonMapperProvider), but we only have custom (de)serialization for the "_id" field as far I could see and nothing generic for ObjectId, or did I miss something?
There was a problem hiding this comment.
Aha, I missed that. The DefaultJacksonMapperProvider registers an _idSerializer that serializes ObjectId to String. Maybe it already does the job?
There was a problem hiding this comment.
you're rigth it is already covered by the DefaultJacksonMapperProvider so I now removed the special handling here.
| @Override | ||
| public Filter visitIncludesExpr(OQLParser.IncludesExprContext ctx) { | ||
| String field = transform(unescapeStringIfNecessary(ctx.expr(0).getText())); | ||
| if (ignoreAttributes.contains(field)) { |
There was a problem hiding this comment.
What is the purpose of ignoreAttributes and where is it used?
There was a problem hiding this comment.
This isn't new code, as far as I could see it transforms filter on specific ignored fields as no-op. It seems to be used when falling back on RAW measurements where some attirbutes doesn't exists, Added some general javadoc on the class.
2 participants
No description provided.