[controller][server][vpj][samza][producer] Use lazy initialization of producer in VeniceWriter #713

sushantmane · 2023-10-25T05:33:41Z

Use lazy initialization of producer in VeniceWriter

My old PR #224 removed lazy producer init in VWFactory.
Bringing it back.

How was this PR tested?

CI

Does this PR introduce any user-facing changes?

No. You can skip the rest of this section.
Yes. Make sure to explain your proposed changes and call out the behavior change.

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java

sixpluszero

LGTM except above clarification.

lluwm · 2023-10-25T17:44:34Z

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java

        OPEN_VENICE_WRITER_COUNT.decrementAndGet();
      } catch (Exception e) {
        logger.warn("Swallowed an exception while trying to close the VeniceWriter for topic: {}", topicName, e);
        VENICE_WRITER_CLOSE_FAILED_COUNT.incrementAndGet();
      }
-      threadPoolExecutor.shutdown();
+      getThreadPoolExecutor().shutdown();


This might need a bit improvement, shutdown only makes sure no new tasks will be accepted. Take a look at our conventions for close a thread pool, e.g., VeniceParentHelixAdmin.close or somewhere else.

Do you mean use shutdownNow to interrupt the running threads?

yes, shutdownnow + awaitTermination for a limited timeout. does it make any sense?

lluwm · 2023-10-25T17:44:44Z

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java

          OPEN_VENICE_WRITER_COUNT.decrementAndGet();
        } catch (Exception e) {
          logger.warn("Swallowed an exception while trying to close the VeniceWriter for topic: {}", topicName, e);
          VENICE_WRITER_CLOSE_FAILED_COUNT.incrementAndGet();
        }
-        threadPoolExecutor.shutdown();
+        getThreadPoolExecutor().shutdown();


FelixGV · 2023-10-25T17:44:45Z

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java

@@ -87,7 +88,7 @@ public class VeniceWriter<K, V, U> extends AbstractVeniceWriter<K, V, U> {
      new ChunkedPayloadAndManifest(null, null);

  // use for running async close and to fetch number of partitions with timeout from producer
-  private final ThreadPoolExecutor threadPoolExecutor;
+  private final Lazy<ThreadPoolExecutor> threadPoolExecutorLazy = Lazy.of(this::createThreadPoolExecutor);


Should we make this static? Do we really want one thread pool per VW? We can have a lot of VW per server...

But with single pool, you won't be able to limit the thread count per VW? Or is it still possible?

The limit would need to be higher, but do we really need some per-VW threads? If we have 1K VWs in a server, do we really want 2K threads spread across 1K TPs? Would it be enough to have a single static TP with, e.g., max 100 threads, and other tasks get queued if we tap out all threads?

Hmm, yeah that's probably more reasonable.
I thought the 2 per VW is required for XInfra purpose. Maybe @sushantmane knows better about this...

Originally I was thinking about having just one ThreadPool for all VWs. But I wanted let callers's have some control over the concurrency of close operations which is where this thread pool is primarily used. Making this a static pool would mean that the concurrency isn't fully controlled by the callers.

Ah I see... so I guess it could be fine then...

@FelixGV - Should we merge this PR then?

I'm changing the scope of this PR slightly:

Keep ThreadPoolExecutor per VW for now

Keep lazy init of PubSubProducer and ThreadPool

Close segments concurrently (VW::endAllSegments)

Do let me know if there are other things that I should address. Thanks.

Update PR with the above changes.

Thanks, I left another review.

nisargthakkar · 2023-10-26T21:08:25Z

Since this touches VW, can you update the components affected to include everything that uses VW? (or use all) since probably only routers, tc and fc are excluded

controller, server, dvc, vpj, samza, producer, ...

FelixGV

Left some minor comments. Thanks for working on this.

FelixGV · 2023-10-27T16:08:27Z

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriterFactory.java

@@ -41,6 +43,7 @@ public <K, V, U> VeniceWriter<K, V, U> createVeniceWriter(VeniceWriterOptions op
    return new VeniceWriter<>(
        options,
        props,
-        producerAdapterFactory.create(props, options.getTopicName(), options.getBrokerAddress()));
+        Lazy.of(() -> producerAdapterFactory.create(props, options.getTopicName(), options.getBrokerAddress())),


I'm curious about this... have you found specific code paths that were instantiating a VW but then not actually using it to produce anything?

In many places, we are already wrapping the VW into Lazy, or else use other patterns of lazy init (e.g. computeIfAbsent). So now we'll be doing double-lazy wrapping in many places. In theory it's fine but it seems a bit dirty to me... maybe the clean thing to do is to remove the "outer" lazy wrapping and just keep this inner one, or perhaps to make the remaining unwrapped VW into Lazy<VW> (and avoid the inner lazy). I don't have a strong opinion either way but I feel like it's something we should consider.

Anyway, I don't mean to say you should embark on a big refactoring in this PR, but I am interested in hearing your opinion on the above... I imagine you looked at this code more recently than I did if you're doing this, so maybe you caught some interesting context IDK about along the way.

You're right. I just double-checked the code, and, except for two place (and none of them are in server startup path), we're indeed initializing VW only when we need it. One trigger to add lazy initialization for VW's producer was based on hazy log reading, where I saw producers being one after another during startup . I should have properly checked the code before adding this code.

Will update the PR to remove lazy init. Thanks!

...e-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/MetaSystemStoreTest.java

FelixGV · 2023-10-27T16:12:29Z

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java

+  private PubSubProducerAdapter getProducerAdapter() {
+    return producerAdapterLazy.get();
+  }
+
+  private ThreadPoolExecutor getThreadPoolExecutor() {
+    return threadPoolExecutorLazy.get();
+  }


I think personally I would prefer not hiding usages of Lazy in this way. I prefer letting the producerAdapterLazy.get() be present at the call site, as it gives maintainers a nudge to think about whether they really want to initialize the Lazy instance. In some cases, it may be desirable to only use the lazy if it's already initialized, by calling ifPresent(instance -> { ... }) on it.

That makes sense. Will update the PR.

FelixGV · 2023-10-27T16:14:38Z

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java

+        getProducerAdapter().close(topicName, closeTimeOutInMs, gracefulClose);
        OPEN_VENICE_WRITER_COUNT.decrementAndGet();
      } catch (Exception e) {
        logger.warn("Swallowed an exception while trying to close the VeniceWriter for topic: {}", topicName, e);
        VENICE_WRITER_CLOSE_FAILED_COUNT.incrementAndGet();
      }
-      threadPoolExecutor.shutdown();
+      getThreadPoolExecutor().shutdown();


Wrap inside ifPresent rather than doing a late initialization just for the sake of tearing it down right after...

Good catch! Thanks

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java

sushantmane · 2023-11-06T08:07:25Z

I'm gonna put it in draft mode because I want to take some time to think it over and make the PR better.

sushantmane · 2023-11-06T08:13:22Z

I'm gonna put it in draft mode because I want to take some time to think it over and make the PR better.

PR #720 has substantially reduced the number of MetaStore VW's per host. I'm considering updating the PR based on Felix's suggestion to use a single thread pool for all VW's instead of a thread pool per VW.

I would also like to consider sending CMs concurrently.

sushantmane · 2024-02-29T22:02:14Z

Closing this PR for now. I would like to complete it at some point..

sixpluszero reviewed Oct 25, 2023

View reviewed changes

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java Show resolved Hide resolved

sixpluszero previously approved these changes Oct 25, 2023

View reviewed changes

lluwm reviewed Oct 25, 2023

View reviewed changes

FelixGV reviewed Oct 25, 2023

View reviewed changes

sushantmane marked this pull request as draft October 25, 2023 18:51

sushantmane added 2 commits October 26, 2023 16:20

Lazy init in VeniceWriter

8f7343d

Close segments concurrently

04e94b3

sushantmane dismissed sixpluszero’s stale review via 04e94b3 October 27, 2023 11:21

sushantmane force-pushed the li-fix-flakies branch from 726638b to 04e94b3 Compare October 27, 2023 11:21

sushantmane changed the title ~~[controller][server] Use lazy initialization of producer in VeniceWriter~~ [controller][server][vpj][samza][producer] Use lazy initialization of producer in VeniceWriter Oct 27, 2023

sushantmane marked this pull request as ready for review October 27, 2023 11:24

FelixGV reviewed Oct 27, 2023

View reviewed changes

nisargthakkar reviewed Oct 27, 2023

View reviewed changes

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java Show resolved Hide resolved

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java Show resolved Hide resolved

sushantmane closed this Nov 3, 2023

sushantmane deleted the li-fix-flakies branch November 3, 2023 02:44

sushantmane restored the li-fix-flakies branch November 3, 2023 02:44

sushantmane reopened this Nov 3, 2023

sushantmane marked this pull request as draft November 6, 2023 08:07

sushantmane closed this Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[controller][server][vpj][samza][producer] Use lazy initialization of producer in VeniceWriter #713

[controller][server][vpj][samza][producer] Use lazy initialization of producer in VeniceWriter #713

sushantmane commented Oct 25, 2023 •

edited

Loading

sixpluszero left a comment

lluwm Oct 25, 2023

sushantmane Oct 27, 2023

lluwm Nov 6, 2023

lluwm Oct 25, 2023

FelixGV Oct 25, 2023

sixpluszero Oct 25, 2023

FelixGV Oct 25, 2023

sixpluszero Oct 25, 2023

sushantmane Oct 25, 2023 •

edited

Loading

FelixGV Oct 25, 2023

sushantmane Oct 25, 2023

sushantmane Oct 26, 2023 •

edited

Loading

sushantmane Oct 27, 2023

FelixGV Oct 27, 2023

nisargthakkar commented Oct 26, 2023

FelixGV left a comment

FelixGV Oct 27, 2023 •

edited

Loading

sushantmane Oct 28, 2023

FelixGV Oct 27, 2023

sushantmane Oct 28, 2023 •

edited

Loading

FelixGV Oct 27, 2023

sushantmane Oct 28, 2023

sushantmane commented Nov 6, 2023

sushantmane commented Nov 6, 2023 •

edited

Loading

sushantmane commented Feb 29, 2024

[controller][server][vpj][samza][producer] Use lazy initialization of producer in VeniceWriter #713

[controller][server][vpj][samza][producer] Use lazy initialization of producer in VeniceWriter #713

Conversation

sushantmane commented Oct 25, 2023 • edited Loading

Use lazy initialization of producer in VeniceWriter

How was this PR tested?

Does this PR introduce any user-facing changes?

sixpluszero 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

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

sushantmane Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushantmane Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisargthakkar commented Oct 26, 2023

FelixGV left a comment

Choose a reason for hiding this comment

FelixGV Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushantmane Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushantmane commented Nov 6, 2023

sushantmane commented Nov 6, 2023 • edited Loading

sushantmane commented Feb 29, 2024

sushantmane commented Oct 25, 2023 •

edited

Loading

sushantmane Oct 25, 2023 •

edited

Loading

sushantmane Oct 26, 2023 •

edited

Loading

FelixGV Oct 27, 2023 •

edited

Loading

sushantmane Oct 28, 2023 •

edited

Loading

sushantmane commented Nov 6, 2023 •

edited

Loading