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

Add isolation option to the vault lock #754

Merged
merged 2 commits into from
Feb 10, 2025
Merged

Conversation

alexandrosfilios
Copy link
Contributor

No description provided.

Copy link
Contributor

@adecaro adecaro left a comment

Choose a reason for hiding this comment

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

Please, clarify the points raised.

@alexandrosfilios alexandrosfilios force-pushed the f-rwset-isolation branch 4 times, most recently from 73541a3 to f3d4c82 Compare February 7, 2025 15:13
@adecaro
Copy link
Contributor

adecaro commented Feb 8, 2025

@alexandrosfilios, can you add a performance test to measure how many transaction per second you can commit while producing RWSets?

@alexandrosfilios alexandrosfilios force-pushed the f-rwset-isolation branch 2 times, most recently from b096055 to 61be65e Compare February 8, 2025 09:47
@alexandrosfilios
Copy link
Contributor Author

@alexandrosfilios, can you add a performance test to measure how many transaction per second you can commit while producing RWSets?

Yes writing perf tests as part of another pr

@@ -82,15 +82,6 @@ func NewInterceptor[V driver.ValidationCode](
}

func (i *Interceptor[V]) IsValid() error {
tx, err := i.vaultStore.GetTxStatus(context.Background(), i.txID)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this check not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the db makes the check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this method is only needed in tests

@@ -51,18 +51,13 @@ func newTxCodePersistence(readDB *sql.DB, writeDB common.WriteDB, tables common.

func (db *VaultPersistence) Store(ctx context.Context, txIDs []driver.TxID, writes driver.Writes, metaWrites driver.MetaWrites) error {
span := trace.SpanFromContext(ctx)
span.AddEvent("start_store")
defer span.AddEvent("end_store")
span.AddEvent("Start store")
Copy link
Contributor

Choose a reason for hiding this comment

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

the rest of the code still uses the convention of separating words with _ and keeping everything lowercase. Was this intentional?

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 I am changing them all slowly because it is more readable on the UI side of Jaeger

@adecaro adecaro self-requested a review February 10, 2025 12:05
Copy link
Contributor

@adecaro adecaro left a comment

Choose a reason for hiding this comment

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

I left some questions to be checked just to make sure the semantic is still correct. I think an interface is not needed anymore.

Signed-off-by: Alexandros Filios <[email protected]>
@alexandrosfilios alexandrosfilios merged commit 90e0fbb into main Feb 10, 2025
19 of 20 checks passed
@alexandrosfilios alexandrosfilios deleted the f-rwset-isolation branch February 10, 2025 15:38
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.

2 participants