Skip to content

Feature/spline 954 acid tx#1107

Merged
wajda merged 43 commits into
developfrom
feature/spline-954-acid-tx
Oct 14, 2022
Merged

Feature/spline 954 acid tx#1107
wajda merged 43 commits into
developfrom
feature/spline-954-acid-tx

Conversation

@wajda
Copy link
Copy Markdown
Contributor

@wajda wajda commented Sep 28, 2022

Fixes #954

  1. Removed arangodb-AQL-functions module and moved the code to the arangodb-foxx-services
  2. Moved custom AQL functions lifecycle management responsibility from the Admin CLI to the Foxx setup/teardown scripts.
  3. Moved lineage ingestion functionality from the Producer service layer to the Foxx layer
  4. Changed the lineage persistence logic according to the proposed application level ACID Tx protocol.

@wajda wajda force-pushed the feature/spline-954-acid-tx branch from bb321fe to 5039bcb Compare September 28, 2022 17:19
Comment thread arangodb-foxx-api/scala-ts.conf Outdated


export function getExecutionEventFromEventKey(eventKey: DocumentKey): ExecutionEvent {
export function getExecutionEventFromEventKey(eventKey: DocumentKey): Progress {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the reason behind this change ExecutionEvent -> Progress ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just eliminating duplicated type definition. More changes are yet to be made to the model as different parts of it have being moved from other modules and consolidated on the Foxx layer.

@@ -0,0 +1,56 @@
/*
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the meaning of the d int the name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was struggling to find an understandable name for the directory and decided to just name it after the type of the files it contains. It's temporary here, I'm going to remove it as soon as it's promoted to the DefinitelyTyped project.

.d.ts - is a standard TypeScript type declaration file extension. In this case it's extra definition of the Foxx API that are missing form the latest version of the @types/arangodb package

.generateJs()

generatedJS should be {
generatedJS shouldEqual {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is the reason for this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

consistency. The other spec statements in the file use shouldEqual. Moreover to me it seems more appropriate and for the string comparison assertion, while be implies a predicate following it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There seems to be a functional difference between equal and be.
AS I understand it be always works the same as == in Scala, whereas equal will use a user-defined implicit Equality if available in scope.

Maybe it would be better to use be as a default and equal only when we need the additional functionality?

https://www.scalatest.org/user_guide/using_matchers#checkingEqualityWithMatchers

Copy link
Copy Markdown
Contributor Author

@wajda wajda Oct 13, 2022

Choose a reason for hiding this comment

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

We could. I don't see too much practical difference to be honest, but I'm fine with either. I more care about consistency though, so that the same logic of choice is applied throughout the whole code base. We can discuss it on CQC.

wajda added 28 commits October 4, 2022 20:07
…eation, replace UUIDv4 TxId with ArangoDB autogen key.
@wajda wajda force-pushed the feature/spline-954-acid-tx branch from e4a1838 to e583424 Compare October 4, 2022 18:08
@wajda wajda force-pushed the feature/spline-954-acid-tx branch from 2a9aa14 to 07bb430 Compare October 5, 2022 16:41
@wajda wajda force-pushed the feature/spline-954-acid-tx branch from 5a55284 to 1162bf2 Compare October 6, 2022 13:27
@wajda wajda marked this pull request as ready for review October 10, 2022 15:04
@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

RETURN DISTINCT readEvent
`

Logger.debug(query)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@wajda debug

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

... so?

FOR p IN allObservedEvents RETURN p
`

Logger.debug(query)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@wajda do you really want to leave it in the codebase?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, why?

@@ -0,0 +1,337 @@
/*
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@wajda is it typings for the fs module?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if yes, I would recommend leaving @types/node in package.json and in the tsconfig.json
https://stackoverflow.com/questions/43048113/use-fs-in-typescript

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried that but unfortunately it didn't work. ArangoDB for some reason uses a slightly different File system API.

The implementation tries to follow the CommonJS Filesystem/A/0 specification where possible.

Not sure how many specs for the FS module exist in the JavaScript world, but the one @types/node provides doesn't fit what ArangoDB is using. My intention for this type-def file was to replace it with a library one as soon as possible (either I find something existing, or make a PR to the @types/arangodb)

@@ -0,0 +1,49 @@
/*
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@wajda I would recommend reorganizing the number of subfolders, due to the complexity and avoiding the Doorway Effect

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure! Would you suggest anything specific? :)

Copy link
Copy Markdown
Contributor

@dk1844 dk1844 left a comment

Choose a reason for hiding this comment

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

Read the code, have not found anything suspicious. But I must admit that I do not understand every bit of the code.

@wajda wajda merged commit ab09c35 into develop Oct 14, 2022
@wajda wajda deleted the feature/spline-954-acid-tx branch October 14, 2022 15:41
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.

Optimize transaction size + ACID on cluster

4 participants