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

Update cache SPI and resource serialization #92

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wzy1935
Copy link
Contributor

@wzy1935 wzy1935 commented Jul 15, 2024

Closes #67

Updated the cache SPI and added serialization for the Resource class.

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thank you @wzy1935 ! I did a first round of comments

src/main/java/io/vertx/httpproxy/spi/cache/Cache.java Outdated Show resolved Hide resolved
src/main/java/io/vertx/httpproxy/cache/CacheOptions.java Outdated Show resolved Hide resolved
src/main/java/io/vertx/httpproxy/impl/CacheImpl.java Outdated Show resolved Hide resolved
src/main/java/io/vertx/httpproxy/spi/cache/Resource.java Outdated Show resolved Hide resolved
@wzy1935 wzy1935 mentioned this pull request Jul 24, 2024
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thank you @wzy1935 , I've added another batch of comments

Can you also add a test that verifies the behavior of a shared cached? That is, if it has stored a response for a one reverse proxy instance, another proxy instance will not have to contact the backend.

src/main/java/io/vertx/httpproxy/cache/CacheOptions.java Outdated Show resolved Hide resolved
src/main/java/io/vertx/httpproxy/HttpProxy.java Outdated Show resolved Hide resolved
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thank you @wzy1935 ! Some more comments

@tsegismont
Copy link
Contributor

Can you please rebase this PR? Thank you

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thank you @wzy1935 , another batch of reviews

src/main/java/io/vertx/httpproxy/HttpProxy.java Outdated Show resolved Hide resolved
src/main/java/io/vertx/httpproxy/cache/CacheOptions.java Outdated Show resolved Hide resolved

@Override
public Future<Void> close() {
return Future.succeededFuture();
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't talk about clearing the map on close, did we? Would it be necessary in your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I also think there's no need, but when I called the vertx.createSharedResource() in ReverseProxy#newCache, there's a closeFuture that I don't know how to deal with it.

Maybe you could check if the following would work? I think I should change to this version:

  // ReverseProxy#newCache
  public Cache newCache(CacheOptions options, Vertx vertx) {
    if (options.getShared()) {
      CloseFuture closeFuture = new CloseFuture();
      return ((VertxInternal) vertx).createSharedResource("__vertx.shared.proxyCache", options.getName(), closeFuture, (cf_) -> {
        Cache cache = new CacheImpl(options);
        return cache;
      });
    }
    return new CacheImpl(options);
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

The close future is not bound to anything, neither Vert.x nor the context. Take a look at how Vert.x creates shared http clients, or shared Vert.x SQL Clients, shared Kafka Producers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this correct then?

  public Cache newCache(CacheOptions options, Vertx vertx) {
    if (options.getShared()) {
      VertxInternal vx = (VertxInternal) vertx;
      CloseFuture closeFuture = new CloseFuture();
      Cache cache = vx.createSharedResource("__vertx.shared.proxyCache", options.getName(), closeFuture, (cf_) -> {
        return new CacheImpl(options);
      });
      ContextInternal ctx = vx.getContext();
      if (ctx != null) {
        ctx.addCloseHook(closeFuture);
      } else {
        vx.addCloseHook(closeFuture);
      }
      return cache;
    }
    return new CacheImpl(options);
  }

So now I know the callback (the cf_ one) doesn't need to close anything because there's nothing to close for the Cache interface; and for the outside closeFuture, my current understanding is that by calling closeFuture.close() means declaring "giving up the reference holding" of this shared resource and reduce the reference counter (Hook.count) by 1.

Correct me if I'm wrong: the outside closeFuture still should be managed because we want to maintain the reference counter correct, and when the vertx.close() is called, the counter will be cleared to 0 even if newCache being called multiple times (because addCloseHook is registered multiple times as well).

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.

Introduce a cache storage SPI
4 participants