Optimize tuple management #65

sbckr · 2024-08-26T07:23:10Z

No description provided.

- intra-vcp/tuples endpoint now returns bytes instead of JSON (breaking change) - tupleList is serialized to bytes Signed-off-by: Alwin Zomotor <[email protected]>

- Castor no longer uses the TupleList class and directly returns bytes retrieved from MinIO Signed-off-by: Alwin Zomotor <[email protected]>

Signed-off-by: Alwin Zomotor <[email protected]>

codecov · 2024-08-26T07:25:02Z

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.24%. Comparing base (eead173) to head (31376bc).

Files	Patch %	Lines
...carbynestack/castor/common/entities/TupleList.java	0.00%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #65      +/-   ##
============================================
- Coverage     85.07%   83.24%   -1.83%     
+ Complexity      300      286      -14     
============================================
  Files            56       56              
  Lines          1045      979      -66     
  Branches         70       63       -7     
============================================
- Hits            889      815      -74     
- Misses          125      133       +8     
  Partials         31       31

Flag	Coverage Δ
common	`73.68% <0.00%> (-1.77%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...stack/castor/common/entities/ArrayBackedTuple.java	`94.11% <ø> (ø)`
...r/client/download/DefaultCastorIntraVcpClient.java	`92.30% <ø> (-0.68%)`	⬇️
...castor/service/config/CastorServiceProperties.java	`91.66% <ø> (ø)`
...service/download/DefaultTuplesDownloadService.java	`93.54% <ø> (-1.69%)`	⬇️
...e/persistence/cache/CreateReservationSupplier.java	`100.00% <ø> (ø)`
...e/persistence/cache/ReservationCachingService.java	`94.89% <ø> (-0.87%)`	⬇️
...e/persistence/cache/UnlockReservationSupplier.java	`100.00% <ø> (ø)`
.../persistence/cache/WaitForReservationCallable.java	`72.22% <ø> (-1.47%)`	⬇️
...stence/fragmentstore/TupleChunkFragmentEntity.java	`92.50% <ø> (-0.36%)`	⬇️
...ragmentstore/TupleChunkFragmentStorageService.java	`91.52% <ø> (-0.42%)`	⬇️
... and 5 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eead173...31376bc. Read the comment docs.

sbckr · 2024-09-13T06:11:36Z

There should be separate branches for the individual features

sbckr

I stopped the review halfway through.

Please apologize, but it looks a bit messy. Please separate the features into individual branches and clean up the code.

I will continue afterwards.
Thank you

sbckr · 2024-09-16T08:55:41Z

castor-common/src/main/java/io/carbynestack/castor/common/entities/ArrayBackedTuple.java

@@ -8,6 +8,8 @@
 package io.carbynestack.castor.common.entities;

 import io.carbynestack.castor.common.exceptions.CastorClientException;
+
+import java.io.ByteArrayOutputStream;


unused import?

If not update Copyright Header

sbckr · 2024-09-16T08:56:11Z

castor-common/src/main/java/io/carbynestack/castor/common/entities/TupleList.java

Please update Copyright header

sbckr · 2024-09-16T09:01:31Z

castor-common/src/main/java/io/carbynestack/castor/common/entities/TupleList.java

@@ -70,6 +70,19 @@ public static <T extends Tuple<T, F>, F extends Field> TupleList<T, F> fromStrea
    }
  }

+  public byte[] toByteArray(){


This duplicates code from asChunk(UUID).
Please re-use this method accordingly.

sbckr · 2024-09-16T10:00:37Z

...client/src/main/java/io/carbynestack/castor/client/download/DefaultCastorIntraVcpClient.java

Please update Copyright header

sbckr · 2024-09-16T10:13:02Z

...client/src/main/java/io/carbynestack/castor/client/download/DefaultCastorIntraVcpClient.java

+                      byte[].class)
+              .get();
+      long length = tupleType.getTupleSize() * count;
+      return TupleList.fromStream(tupleType.getTupleCls(),


As Tuples as mostly consumed within a stream, it may make sense to keep tuples as a stream (potentially still wrapped as a TupleList and lazily converted later if required).

maybe TupleList became obsolete

sbckr · 2024-09-16T12:54:21Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/ReservationCachingService.java

+   * @throws InterruptedException : If the fragments can not be reserved
+   */
+  @Transactional()
+  public void storeReservationInDB(@NotNull Reservation reservation) throws InterruptedException {


Am I right that this method requires the reservation elements to be ordered by number of reserved Tuples where all elements with number of tuples < initial fragment size must be listed first.
That feels fragile especially as not properly documented (is it?)

sbckr · 2024-09-16T12:56:11Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/UnlockReservationSupplier.java

@@ -13,6 +13,8 @@
 import io.carbynestack.castor.common.exceptions.CastorClientException;
 import io.carbynestack.castor.common.exceptions.CastorServiceException;
 import java.util.function.Supplier;
+
+import io.micrometer.core.annotation.Timed;


unused import

sbckr · 2024-09-16T12:56:54Z

...c/main/java/io/carbynestack/castor/service/persistence/cache/WaitForReservationCallable.java

Update Copyright

sbckr · 2024-09-16T12:57:49Z

.../java/io/carbynestack/castor/service/persistence/fragmentstore/TupleChunkFragmentEntity.java

@@ -62,6 +62,11 @@ public class TupleChunkFragmentEntity implements Serializable {
  static final String FRAGMENT_LENGTH_COLUMN = "fragment_length";
  static final String ACTIVATION_STATUS_COLUMN = "activation_status";
  static final String RESERVATION_ID_COLUMN = "reservation_id";
+  static final String VIEW_NAME = "distributed_fragments";
+  static final String POD_HASH_FIELD = "pod_hash";


some unused statics

sbckr · 2024-09-16T12:57:56Z

.../java/io/carbynestack/castor/service/persistence/fragmentstore/TupleChunkFragmentEntity.java

update copyright

Signed-off-by: Alwin Zomotor <[email protected]>

- transaction between two threads - now no longer checks for available tuples and will fail when trying to reserve instead, saving lots of time. Signed-off-by: Alwin Zomotor <[email protected]>

- improved reservation locking, still needds to be fixed Signed-off-by: Alwin Zomotor <[email protected]>

Signed-off-by: Alwin Zomotor <[email protected]>

- now splits tuplechunks correctly if number of tuples % fragmentsize > 0. Signed-off-by: Alwin Zomotor <[email protected]>

Signed-off-by: Alwin Zomotor <[email protected]>

sbckr · 2024-09-18T11:07:28Z

...vice/src/main/java/io/carbynestack/castor/service/download/DefaultTuplesDownloadService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021 - for information on the respective copyright owner
+ * Copyright (c) 2024 - for information on the respective copyright owner


Copyright must be in format

...
Copyright (c) 2021 - 2024 - for information on the respective copyright owner
...

-> Copyright (c) [Year of file added] - [Year last modified] ...
if [Year of file added] is the same as [Year last modified] it is simply
-> Copyright (c) [Year of file added] ...

sbckr · 2024-09-18T13:08:16Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/CreateReservationSupplier.java

+              .retrieveSinglePartialFragment(
+                  tupleType, oddToReserve < castorServiceProperties.getInitialFragmentSize())
+              .orElseThrow(
+                  () -> new CastorServiceException(FAILED_FETCH_AVAILABLE_FRAGMENT_EXCEPTION_MSG));


Is there a test that ensures that all previous reservations are rolled back if there aren't enough tuples available

sbckr

see individual comments

sbckr · 2024-09-18T13:45:36Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/CreateReservationSupplier.java

+      TupleChunkFragmentEntity availableFragment =
+          fragmentStorageService
+              .retrieveSinglePartialFragment(
+                  tupleType, oddToReserve < castorServiceProperties.getInitialFragmentSize())


second argument preferSmall of retrieveSinglePartialFragment(TupleType tupleType, boolean preferSmall) is always true, does this make sense?

second argument preferSmall of retrieveSinglePartialFragment(TupleType tupleType, boolean preferSmall) is always true, does this make sense?

This argument is not always true. oddToReserve may be greater than the initial Fragment size, if Castor couldn't get the whole amount of Tuples in whole fragments. This happened in my test setup because Klyshko had an off-by-one error and would save 99,999 Tuples instead of 100,000 Tuples into a tupleChunk resulting in lots of tuples getting saved in partial fragments.

Line 79 adds the amount of tuples to oddToReserve that should have been reserved as round, couldn't.

sbckr · 2024-09-18T14:09:54Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/ReservationCachingService.java

+
+  /**
+   * Maps the reservation elements to tuplefragments and saves them accordingly in batches to the
+   * RDBMS. This function assumes that all 'non-round' fragments are saved before 'round' fragments


what means saved? in the database?

what means saved? in the database?

Yes. Saved means saved in this context (RDBMS) means saved to postgres.

sbckr · 2024-09-19T06:22:09Z

...est/java/io/carbynestack/castor/service/persistence/cache/ReservationCachingServiceTest.java

+    when(dedicatedTransactionServiceOptionalMock.isPresent()).thenReturn(true);
+    when(tupleChunkFragmentStorageServiceMock.retrieveSinglePartialFragment(tupleType, true))
+        .thenReturn(Optional.of(fragmentEntity));
+    // when(castorInterVcpClientSpy.shareReservation(any(Reservation.class))).thenReturn(false);


please keep code clean

CRAlwin added 4 commits July 31, 2024 11:05

perf!(service): Changed serialization of TupleLists

121ccc5

- intra-vcp/tuples endpoint now returns bytes instead of JSON (breaking change) - tupleList is serialized to bytes Signed-off-by: Alwin Zomotor <[email protected]>

perf: Directly return Bytes in intra-vcp/tuples

fb4103b

- Castor no longer uses the TupleList class and directly returns bytes retrieved from MinIO Signed-off-by: Alwin Zomotor <[email protected]>

refactor: Removed the timed annotation previously added

03e5493

Signed-off-by: Alwin Zomotor <[email protected]>

perf!: WIP of new reservation strategy

c47407e

sbckr commented Sep 16, 2024

View reviewed changes

CRAlwin added 8 commits September 18, 2024 11:34

fix: fixed wrong calculation of available fragments

1682218

Signed-off-by: Alwin Zomotor <[email protected]>

fix: reservation-locking:

06a413a

- transaction between two threads - now no longer checks for available tuples and will fail when trying to reserve instead, saving lots of time. Signed-off-by: Alwin Zomotor <[email protected]>

fix: reservation-locking:

67f48a6

- improved reservation locking, still needds to be fixed Signed-off-by: Alwin Zomotor <[email protected]>

fix: all concurrency issues are now resolved!

e103aad

Signed-off-by: Alwin Zomotor <[email protected]>

fix: websocketClient

2045236

- now splits tuplechunks correctly if number of tuples % fragmentsize > 0. Signed-off-by: Alwin Zomotor <[email protected]>

fix: now append round fragments at the end of the list

77ec75a

Signed-off-by: Alwin Zomotor <[email protected]>

fix: now reserves first tuple correctly; fixed some typos

d80e352

Signed-off-by: Alwin Zomotor <[email protected]>

fix: fixed all tests and added documentation

b5d99c3

Signed-off-by: Alwin Zomotor <[email protected]>

CRAlwin force-pushed the optimize-tuple-management branch from 3ed574f to 516901d Compare September 18, 2024 09:35

fix: removed toByteArray function as it is now no longer needed

36badbd

CRAlwin force-pushed the optimize-tuple-management branch from 516901d to 36badbd Compare September 18, 2024 09:36

sbckr commented Sep 18, 2024

View reviewed changes

sbckr commented Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize tuple management #65

Optimize tuple management #65

sbckr commented Aug 26, 2024

codecov bot commented Aug 26, 2024

sbckr commented Sep 13, 2024

sbckr left a comment •

edited

Loading

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 16, 2024

sbckr Sep 18, 2024

sbckr Sep 18, 2024

sbckr left a comment

sbckr Sep 18, 2024

CRAlwin Sep 25, 2024

sbckr Sep 18, 2024

zalwin Sep 25, 2024 •

edited

Loading

sbckr Sep 19, 2024

Optimize tuple management #65

Are you sure you want to change the base?

Optimize tuple management #65

Conversation

sbckr commented Aug 26, 2024

codecov bot commented Aug 26, 2024

Codecov Report

sbckr commented Sep 13, 2024

sbckr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbckr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zalwin Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbckr left a comment •

edited

Loading

zalwin Sep 25, 2024 •

edited

Loading