MSQ worker: Support in-memory shuffles. #16790

gianm · 2024-07-24T10:29:06Z

This patch is a follow-up to #16168, adding worker-side support for in-memory shuffles. Changes include:

Worker-side code now respects the same context parameter "maxConcurrentStages"
that was added to the controller in MSQ controller: Support in-memory shuffles; towards JVM reuse. #16168. The parameter remains undocumented
for now, to give us a chance to more fully develop and test this functionality.
WorkerImpl is broken up into WorkerImpl, RunWorkOrder, and RunWorkOrderListener
to improve readability.
WorkerImpl has a new StageOutputHolder + StageOutputReader concept, which
abstract over memory-based or file-based stage results.
RunWorkOrder is updated to create in-memory stage output channels when
instructed to.
ControllerResource is updated to add /doneReadingInput/, so the controller
can tell when workers that sort, but do not gather statistics, are done reading
their inputs.
WorkerMemoryParameters is updated to consider maxConcurrentStages.

Additionally, WorkerChatHandler is split into WorkerResource, so as to match ControllerChatHandler and ControllerResource.

This patch is a follow-up to apache#16168, adding worker-side support for in-memory shuffles. Changes include: 1) Worker-side code now respects the same context parameter "maxConcurrentStages" that was added to the controller in apache#16168. The parameter remains undocumented for now, to give us a chance to more fully develop and test this functionality. 1) WorkerImpl is broken up into WorkerImpl, RunWorkOrder, and RunWorkOrderListener to improve readability. 2) WorkerImpl has a new StageOutputHolder + StageOutputReader concept, which abstract over memory-based or file-based stage results. 3) RunWorkOrder is updated to create in-memory stage output channels when instructed to. 4) ControllerResource is updated to add /doneReadingInput/, so the controller can tell when workers that sort, but do not gather statistics, are done reading their inputs. 5) WorkerMemoryParameters is updated to consider maxConcurrentStages. Additionally, WorkerChatHandler is split into WorkerResource, so as to match ControllerChatHandler and ControllerResource.

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/RunWorkOrder.java

...-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/ChannelStageOutputReader.java

...-core/multi-stage-query/src/main/java/org/apache/druid/msq/rpc/ResourcePermissionMapper.java

...nsions-core/multi-stage-query/src/main/java/org/apache/druid/msq/rpc/ControllerResource.java

...ulti-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/NilStageOutputReader.java

LakshSingla · 2024-07-25T06:11:03Z

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerClient.java

   */
  void postWorkerError(
-      String workerId,
+      @Nullable String queryId,


Looking at the places where this is called, we pass it the worker's task id. Should we keep the parameter name as is? Also, I was taking a look at the controller chat handler and we don't really use the worker's task id afaict. Maybe we can remove the param altogether, though that would introduce backward compatibility issues.
Also, can it be nullable given that we pass this as a URL param?

Ah, I don't actually need to change this from workerId to queryId right now. This was related to some other stuff I was tinkering with, about supporting multiple queries running on the same worker id, but it isn't important for this PR. I'll revert it.

Fwiw, the server side (ControllerResource) does not read this parameter, so it currently doesn't matter what it is. That's probably why I didn't notice the inconsistency when testing this PR.

LakshSingla · 2024-07-25T06:12:16Z

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerClient.java


-  List<String> getTaskList() throws IOException;
+  List<String> getWorkerIds() throws IOException;


nit: lets add javadoc here, even though it wasn't present in the original code.

Added javadoc.

LakshSingla · 2024-07-25T06:25:45Z

...-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/ChannelStageOutputReader.java

+/**
+ * Reader for {@link ReadableFrameChannel}.
+ *
+ * Because this reader returns an underlying channel directly, it must only be used when it is certain that


If this reader can be used by a single consumer, why is there a requirement for adding synchronization to the methods? Also, we can probably guard the shared states with GuardedBy.

There's a single consumer, but the consumer and producer threads might be different, so synchronization is still needed between consumer and producer.

I added @GuardedBy annotations; good call.

LakshSingla · 2024-07-26T15:45:12Z

...stage-query/src/test/java/org/apache/druid/msq/shuffle/output/ByteChunksInputStreamTest.java

+
+public class ByteChunksInputStreamTest
+{
+  private final List<byte[]> chunks = ImmutableList.of(


Should we add some empty arrays in the middle and end as well?

There can't be empty arrays in production, because chunks.add(bytes) is only called if the length is greater than zero. I added some defensive checks to ByteChunksInputStream to verify the chunks are all non-empty.

LakshSingla · 2024-07-26T15:49:12Z

...-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/ChannelStageOutputReader.java

Curious if we should separate out the state transitions from the actual code. The file itself is fairly easily understandable so I am fine either way.

Personally I think it's ok as-is, since the logic is simple.

LakshSingla · 2024-07-26T16:49:29Z

...tage-query/src/main/java/org/apache/druid/msq/shuffle/output/FutureReadableFrameChannel.java

+      channelFuture.cancel(true);
+      channelFuture.addListener(


Quick question - Why are we attaching a listener after canceling the future? Is it a defensive check for closing the channel if the cancel didn't register "in time"? Also given that this is happening on a single thread would we ever encounter such case when the addListener could get triggered?

Yeah, it's in case of a race where channelFuture resolved after populateChannel() but before the call to cancel. In that case the call to cancel would have no effect, and the listener would fire to clean up the channel.

I don't think this needs to be a listener, really, it could just be in-line. I edited the code to make this logic in-line, and added a comment.

...tage-query/src/main/java/org/apache/druid/msq/shuffle/output/FutureReadableFrameChannel.java

LakshSingla · 2024-07-29T03:51:55Z

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/RunWorkOrder.java

nice refactor!

nit: Since a lot of fields like inputSliceReader are populated after initialization, we could use @MonotonicNonNull for readability and verification

I added @MonotonicNonNull to various fields.

LakshSingla · 2024-07-29T04:48:06Z

...ons-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/RunWorkOrderListener.java

+ * Listener for various things that may happen during execution of {@link RunWorkOrder#start()}. Listener methods are
+ * fired in processing threads, so they must be thread-safe, and it is important that they run quickly.


Isn't all of the MSQ work done in the "processing" threads? Does processing here mean the main thread that forms the MSQ's event loop?

It's just the regular processing threads. The point in the javadocs is mainly that there is not a separate thread pool that runs these things, and if they take too long, they will block processing.

LakshSingla · 2024-07-29T04:59:11Z

...s-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerMemoryParameters.java

qq: Why is bundleCount = numWorkersInJvm + numProcessingThreadsInJvm and not just equal to numProcessingThreadsInJvm.

Since we use maxConcurrentStages in the memory parameters, I wonder if it should be the maxConcurrentStages that the user passes (present code), or if it should be the maximum concurrent stages that can happen given the query structure. For example, if each stage sorts in the query, the maximum concurrent stages that can happen = 1, however, if the user passes it as 10, we would be allocating less memory per stage even though it isn't required.

Also, since it is not a user-facing change, I am debating if we should suggest reducing the maxConcurrentStages in the NotEnoughMemoryFault. We should perhaps mention it in the fault's message if maxConcurrentStages > 1.

qq: Why is bundleCount = numWorkersInJvm + numProcessingThreadsInJvm and not just equal to numProcessingThreadsInJvm.

Added a comment about this:

// One bundle per worker + one per processor. The worker bundles are used for sorting (SuperSorter) and the // processing bundles are used for reading input and doing per-partition processing.

Since we use maxConcurrentStages in the memory parameters, I wonder if it should be the maxConcurrentStages that the user passes (present code), or if it should be the maximum concurrent stages that can happen given the query structure. For example, if each stage sorts in the query, the maximum concurrent stages that can happen = 1, however, if the user passes it as 10, we would be allocating less memory per stage even though it isn't required.

That would be a useful optimization for a future patch, imo.

Also, since it is not a user-facing change, I am debating if we should suggest reducing the maxConcurrentStages in the NotEnoughMemoryFault. We should perhaps mention it in the fault's message if maxConcurrentStages > 1.

I added a mention of reducing maxConcurrentStages if it's greater than 1.

LakshSingla · 2024-07-29T05:57:09Z

...e/multi-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/StageOutputReader.java

+   *
+   * @see org.apache.druid.msq.exec.WorkerImpl#readChannel(StageId, int, long)
+   */
+  ListenableFuture<InputStream> readRemotelyFrom(long offset);


Should it be ReadableFrameChannel instead for having a unified interface that the callers can call?

afaict both readLocally() and readRemotelyFrom() read from the local disk and send it to the caller. Upon reading the names, I assumed that readLocally() reads from the local channels while the readRemotelyFrom() reads from some place present outside the local disk.

As such, both are ways of exposing same stream, but in a different way. If my understanding is correct, should we rename the methods to reflect that?

the Javadoc should state what will happen if two calls are made - at t1: readRemotelyFrom(offset1) and at t2: readRemotelyFrom(offset2) when t1 < t2 and offset1 > offset2. From the implementation of the channel reader, it would throw an error.

Should it be ReadableFrameChannel instead for having a unified interface that the callers can call?

The methods have different returns because they are meant for different uses: the first is for reading remotely (so it must return some bytes that can be sent over HTTP) and the second is for reading locally (so it should return a channel object). For this reason the names also make sense (to me 😄). If you have suggestions for better names LMK, otherwise I can keep them as-is.

I've added additional javadocs to StageOutputReader and also its main implementations (ChannelStageOutputReader and FileStageOutputReader) to clarify the design. Please LMK if they do help in understanding.

the Javadoc should state what will happen if two calls are made - at t1: readRemotelyFrom(offset1) and at t2: readRemotelyFrom(offset2) when t1 < t2 and offset1 > offset2. From the implementation of the channel reader, it would throw an error.

I have added this:

* It is implementation-dependent whether calls to this method must have monotonically increasing offsets. * In particular, {@link ChannelStageOutputReader} requires monotonically increasing offsets, but other * implementations do not.

I've also added details to the javadocs for ChannelStageOutputReader and FileStageOutputReader.

LakshSingla · 2024-07-29T08:41:07Z

...-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/ChannelStageOutputReader.java

+  }
+
+  @Override
+  public synchronized void close()


This should set state = CLOSE

Good point. Added this.

LakshSingla · 2024-07-29T08:43:10Z

...-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/ChannelStageOutputReader.java

+  @Override
+  public synchronized void close()
+  {
+    // Call channel.close() unless readLocally() has been called. In that case, we expect the caller to close it.


Can we unify this logic, i.e. both the local and the remote readers call the close() on the reader.

I thought about this, and looked a bit into refactoring it to accomplish this, but I think it actually does make sense the way it is. The idea is consistently that callers should own the closing of the InputStream and ReadableFrameChannel that they get from the readRemotelyFrom and readLocally methods. Refactoring away from that would make the code here more complex, and also I think would make reasoning about ownership of the channels and streams more complex.

For the ChannelStageOutputReader, the readLocally method effectively transfers ownership of the channel to the caller, which is why it makes the ChannelStageOutputReader's own close method do nothing.

I've added javadocs to StageOutputReader and ChannelStageOutputReader clarifying this.

LakshSingla · 2024-07-29T08:53:11Z

...e/multi-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/StageOutputReader.java

suggstion regarding naming of the class - Output is an overloaded term in MSQ. For example - outputChannel = readableChannel + writeableChannel.
In the context of this class, the StageOutputReader only encapsulates the readability portion. Should we remove output from the name to avoid such a confusion? Something like StageResultsReader and StageResultsHolder.

Ah, the OutputChannel class naming has come up before as being confusing. IMO, if we're going to change the name of something, it should be that class. When it has a writable + readable channel pair, it's really more like a "channel pair" or a "pipe". Maybe it'd make sense to change that to OutputPipe in a follow-up, and have the readOnly() method return List<ReadableFrameChannel>?

At any rate, I don't think we need to let the funny naming of that class affect how we name this one.

LakshSingla · 2024-07-29T08:55:35Z

...e/multi-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/StageOutputReader.java

+   *
+   * @see WorkerOrLocalInputChannelFactory#openChannel(StageId, int, int)
+   */
+  ReadableFrameChannel readLocally();


It should mention that this method can be called only once, while the readRemotelyFrom can be called multiple times with variable offsets.

I have added this:

* It is implementation-dependent whether this method can be called multiple times. In particular, * {@link ChannelStageOutputReader#readLocally()} can only be called one time, but the implementations in * {@link FileStageOutputReader} and {@link NilStageOutputReader} can be called multiple times.

LakshSingla · 2024-07-29T09:04:27Z

...lti-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/FileStageOutputReader.java

+  }
+
+  @Override
+  public void close() throws IOException


Should this method also concern with closing the created randomAccessFile? ChannelStageOutputReader has a weird contract that readLocally() will close the channel directly, while the remote one would (presumably) call close() on the OutputReader. If a similar pattern is applied to the FileStageOutputReader, we might be left with unclosed RandomAccessFile references because the caller called close on the output reader, but no one called close on the input stream.

The idea is the caller owns the returned InputStream, so the caller is responsible for closing it. I've updated the method documentation to clarify this:

* Callers are responsible for closing the returned {@link InputStream}. This input stream may encapsulate * resources that are not closed by this class's {@link #close()} method.

LakshSingla · 2024-07-29T09:11:38Z

...-stage-query/src/main/java/org/apache/druid/msq/shuffle/output/ChannelStageOutputReader.java

+      if (len > 0) {
+        final byte[] bytes = new byte[len];
+        src.get(bytes);
+        chunks.add(bytes);


This makes additional copies of the bytes. I am wondering if
a. there's a way to reduce this additional copy.
b. Does having this copy add any meaningful overhead
c. If there's an overhead, was it present in the original code, or does the refactor introduce it.

my thoughts:

a) I think the copy can be eliminated if we alter FrameFileWriter somehow. It accepts a WritableByteChannel that it writes the frame file to, and the WritableByteChannel method int write(ByteBuffer src) requires copying the bytes from src. (The ByteBuffer object cannot be retained, since the caller might reuse it.) So, we'd need to change FrameFileWriter to have some option other than WritableByteChannel.

b) I haven't done extensive profiling, so I'm not sure. I was planning to do more extensive profiling and look into further perf enhancements after this code lands in master.

c) If there's any overhead, it wasn't present in the original code, because this entire code path is new. This code path is only used for in-memory shuffles. The original code always read from on-disk frame files, & did not generate them on-the-fly.

LakshSingla · 2024-07-29T11:13:21Z

...ions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestWorkerClient.java

  }

  @Override
  public void close()
  {
-    inMemoryWorkers.forEach((k, v) -> v.stopGracefully());
+    inMemoryWorkers.forEach((k, v) -> v.stop());


Curious what prompted this modification

I thought that stopGracefully was not really an appropriate method name to have on Worker. It's too linked to the behavior of tasks, and I wanted to make the Worker interface more easily extensible to workers that aren't tasks.

LakshSingla · 2024-07-29T11:38:44Z

...ore/multi-stage-query/src/main/java/org/apache/druid/msq/kernel/worker/WorkerStagePhase.java

@@ -54,11 +54,12 @@ public boolean canTransitionFrom(final WorkerStagePhase priorPhase)
    @Override
    public boolean canTransitionFrom(final WorkerStagePhase priorPhase)
    {
-      return priorPhase == PRESHUFFLE_WAITING_FOR_RESULT_PARTITION_BOUNDARIES;
+      return priorPhase == PRESHUFFLE_WAITING_FOR_RESULT_PARTITION_BOUNDARIES /* if globally sorting */
+             || priorPhase == READING_INPUT /* if locally sorting */;


Why did this not get caught before this patch?

Before this patch, stages that locally sort would not enter the PRESHUFFLE_WRITING_OUTPUT phase. They would go directly from READING_INPUT to RESULTS_READY. But now, they need to enter PRESHUFFLE_WRITING_OUTPUT so they can send doneReadingInput to the controller.

LakshSingla · 2024-07-29T11:39:45Z

...ore/multi-stage-query/src/main/java/org/apache/druid/msq/kernel/worker/WorkerStagePhase.java

@@ -70,7 +71,7 @@ public boolean canTransitionFrom(final WorkerStagePhase priorPhase)
    @Override
    public boolean canTransitionFrom(final WorkerStagePhase priorPhase)
    {
-      return priorPhase == RESULTS_READY;
+      return priorPhase.compareTo(FINISHED) < 0;


should any stage be allowed to transition to finished, even if it hasn't produced any results?

Yes, this is needed for supporting LIMIT. When a downstream stage applies a LIMIT, it's possible for the upstream to be finished if it hasn't fully generated its output. I added a comment describing this case.

LakshSingla · 2024-07-29T11:44:35Z

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerImpl.java

+        // Workers run all work orders they get. There is not (currently) any limit on the number of concurrent work
+        // orders; we rely on the controller to avoid overloading workers.
+        if (kernel.getPhase() == WorkerStagePhase.NEW
+            && kernelHolders.runningKernelCount() < context.maxConcurrentStages()) {


Workers in RESULTS_COMPLETE running in the MEMORY mode would be consuming a decent portion of the memory storing the result output. Does this condition not overload the memory on the worker because at most 1 stage will be not-running but consuming memory (in leap-frogging)

MEMORY just means the transfer is in-memory. The results aren't completely buffered in memory: just one frame at a time per channel. I added a note about this to OutputChannelMode#MEMORY.

New text:

/** * In-memory output channels. Stage shuffle data does not hit disk. In-memory channels do not fully buffer stage * output. They use a blocking queue; see {@link RunWorkOrder#makeStageOutputChannelFactory()}. * * Because stage output is not fully buffered, this mode requires a consumer stage to run at the same time as its * corresponding producer stage. See {@link ControllerQueryKernelUtils#computeStageGroups} for the logic that * determines when we can use in-memory channels. */ MEMORY("memory"),

LakshSingla

Are there end-to-end unit tests testing out the in-memory shuffles with >1 concurrent stages?

gianm · 2024-07-30T04:43:09Z

Are there end-to-end unit tests testing out the in-memory shuffles with >1 concurrent stages?

There aren't end to end tests yet, just unit tests of some of the important pieces. I was thinking integration tests would be ideal (to ensure all the http endpoints work properly as well) but have not yet created those. I was thinking a good time to do that would be after the initial work is committed, but before it's documented.

LakshSingla · 2024-07-30T15:30:44Z

lgtm 🚀

* MSQ worker: Support in-memory shuffles. This patch is a follow-up to apache#16168, adding worker-side support for in-memory shuffles. Changes include: 1) Worker-side code now respects the same context parameter "maxConcurrentStages" that was added to the controller in apache#16168. The parameter remains undocumented for now, to give us a chance to more fully develop and test this functionality. 1) WorkerImpl is broken up into WorkerImpl, RunWorkOrder, and RunWorkOrderListener to improve readability. 2) WorkerImpl has a new StageOutputHolder + StageOutputReader concept, which abstract over memory-based or file-based stage results. 3) RunWorkOrder is updated to create in-memory stage output channels when instructed to. 4) ControllerResource is updated to add /doneReadingInput/, so the controller can tell when workers that sort, but do not gather statistics, are done reading their inputs. 5) WorkerMemoryParameters is updated to consider maxConcurrentStages. Additionally, WorkerChatHandler is split into WorkerResource, so as to match ControllerChatHandler and ControllerResource. * Updates for static checks, test coverage. * Fixes. * Remove exception. * Changes from review. * Address static check. * Changes from review. * Improvements to docs and method names. * Update comments, add test. * Additional javadocs. * Fix throws. * Fix worker stopping in tests. * Fix stuck test.

This reverts commit 01f6cfc.

github-actions bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Jul 24, 2024

Updates for static checks, test coverage.

b6016a2

github-advanced-security bot found potential problems Jul 24, 2024

View reviewed changes

gianm added 2 commits July 24, 2024 12:07

Fixes.

b1445ff

Remove exception.

b22b30e

LakshSingla reviewed Jul 26, 2024

View reviewed changes

Changes from review.

4d8d1a0

github-advanced-security bot found potential problems Jul 26, 2024

View reviewed changes

...tage-query/src/main/java/org/apache/druid/msq/shuffle/output/FutureReadableFrameChannel.java Fixed Show resolved Hide resolved

Address static check.

aa3d3bf

LakshSingla reviewed Jul 29, 2024

View reviewed changes

gianm added 4 commits July 29, 2024 11:43

Changes from review.

11ea1aa

Improvements to docs and method names.

61c337b

Update comments, add test.

112bd71

Additional javadocs.

193be9e

gianm added 2 commits July 29, 2024 22:11

Fix throws.

f9f9321

Fix worker stopping in tests.

ac49b8e

LakshSingla approved these changes Jul 30, 2024

View reviewed changes

Fix stuck test.

ff34ede

gianm merged commit 01f6cfc into apache:master Jul 31, 2024
88 checks passed

gianm deleted the msq-memory-shuffles branch July 31, 2024 01:41

asdf2014 added the Release Notes label Jul 31, 2024

LakshSingla added a commit to LakshSingla/druid that referenced this pull request Aug 9, 2024

Revert "MSQ worker: Support in-memory shuffles. (apache#16790)"

86f52b0

This reverts commit 01f6cfc.

adarshsanjeev mentioned this pull request Aug 22, 2024

Use controller ID while reading from durable storage #16943

Merged

10 tasks

gianm mentioned this pull request Sep 24, 2024

Interactive MSQ profile (Dart) #17139

Open

kfaraz added this to the 31.0.0 milestone Oct 4, 2024


		List<String> getTaskList() throws IOException;
		List<String> getWorkerIds() throws IOException;

		* Listener for various things that may happen during execution of {@link RunWorkOrder#start()}. Listener methods are
		* fired in processing threads, so they must be thread-safe, and it is important that they run quickly.

MSQ worker: Support in-memory shuffles. #16790

MSQ worker: Support in-memory shuffles. #16790

Conversation

gianm commented Jul 24, 2024

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

LakshSingla Jul 29, 2024 • 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

LakshSingla Jul 29, 2024 • 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

LakshSingla Jul 29, 2024 • 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

LakshSingla Jul 29, 2024 • 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

LakshSingla Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LakshSingla Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LakshSingla Jul 29, 2024 • 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

LakshSingla left a comment

Choose a reason for hiding this comment

gianm commented Jul 30, 2024

LakshSingla commented Jul 30, 2024

LakshSingla Jul 29, 2024 •

edited

Loading

LakshSingla Jul 29, 2024 •

edited

Loading

LakshSingla Jul 29, 2024 •

edited

Loading

LakshSingla Jul 29, 2024 •

edited

Loading

LakshSingla Jul 29, 2024 •

edited

Loading

LakshSingla Jul 29, 2024 •

edited

Loading

LakshSingla Jul 29, 2024 •

edited

Loading