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

[#28822] Don't advertise temporary references in the compression table #28823

Merged

Conversation

manuelbernhardt
Copy link
Contributor

@manuelbernhardt manuelbernhardt commented Mar 26, 2020

Fix based on the path, not sure if there's a cleaner way

References #28822

@akka-ci
Copy link

@akka-ci akka-ci commented Mar 26, 2020

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

Copy link
Member

@patriknw patriknw left a comment

thanks for finding and suggesting fix

@@ -620,6 +620,10 @@ private[remote] class Decoder(
(logic, logic)
}

protected def shouldCount(v: InternalActorRef): Boolean =
// don't count temporary refs
!v.path.elements.headOption.contains("temp")
Copy link
Member

@patriknw patriknw Mar 26, 2020

Choose a reason for hiding this comment

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

A path always has at least one element so head would be fine, but elements is probably too costly here.
root is better, but still uncertain of the performance impact of doing this for all messages.

Are these not always PromiseActorRef? isInstanceOf would be better

Copy link
Contributor Author

@manuelbernhardt manuelbernhardt Mar 26, 2020

Choose a reason for hiding this comment

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

@patriknw I tried matching based on instance like here but that doesn't appear to work since at least part of what we're counting is based on decompressed actor refs, so instance checks don't work (not sure what the type of those actor refs is, but it's not the original PromiseActorRef)

Copy link
Contributor Author

@manuelbernhardt manuelbernhardt Mar 26, 2020

Choose a reason for hiding this comment

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

@patriknw new version will try to leverage PromiseActorRef checking for local refs (I think this should work - need to run this later to see if it actually does). Not sure either what's the most cost-effective approach here

Copy link
Member

@patriknw patriknw Mar 26, 2020

Choose a reason for hiding this comment

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

ah, one of them is probably RemoteActorRef

Copy link
Member

@patriknw patriknw Mar 26, 2020

Choose a reason for hiding this comment

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

would an alternative be to always count them but then filter them out when publishing the tables, which is a periodic task?

Copy link
Contributor Author

@manuelbernhardt manuelbernhardt Mar 27, 2020

Choose a reason for hiding this comment

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

that's likely the much sounder approach, I gave it a try in the latest commit

akka-remote/src/main/scala/akka/remote/artery/Codecs.scala Outdated Show resolved Hide resolved
@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Mar 26, 2020
@akka-ci
Copy link

@akka-ci akka-ci commented Mar 26, 2020

Test FAILed.

@manuelbernhardt manuelbernhardt force-pushed the compression_table_temporary_refs branch from 025f45b to e8e4ca2 Compare Mar 26, 2020
@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Mar 26, 2020
@akka-ci
Copy link

@akka-ci akka-ci commented Mar 26, 2020

Test PASSed.

1 similar comment
@akka-ci
Copy link

@akka-ci akka-ci commented Mar 26, 2020

Test PASSed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Mar 27, 2020
@manuelbernhardt manuelbernhardt force-pushed the compression_table_temporary_refs branch from e1648e1 to 30521ff Compare Mar 27, 2020
@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Mar 27, 2020
@akka-ci
Copy link

@akka-ci akka-ci commented Mar 27, 2020

Test FAILed.

@manuelbernhardt manuelbernhardt force-pushed the compression_table_temporary_refs branch from 30521ff to 3309500 Compare Mar 27, 2020
@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Mar 27, 2020
@akka-ci
Copy link

@akka-ci akka-ci commented Mar 27, 2020

Test PASSed.

1 similar comment
@akka-ci
Copy link

@akka-ci akka-ci commented Mar 27, 2020

Test PASSed.

Copy link
Member

@patriknw patriknw left a comment

LGTM, thanks

@@ -192,6 +190,21 @@ private[remote] final class InboundActorRefCompression(
outboundContext.sendControl(
CompressionProtocol.ActorRefCompressionAdvertisement(inboundContext.localAddress, table))
}

override protected def buildTableForAdvertisement(elements: Iterator[ActorRef]): Map[ActorRef, Int] = {
Copy link
Member

@patriknw patriknw Mar 27, 2020

Choose a reason for hiding this comment

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

right, not needed for manifests

Copy link
Member

@johanandren johanandren left a comment

LGTM, thanks @manuelbernhardt !

@johanandren johanandren merged commit 4c81ef8 into akka:master Mar 30, 2020
3 checks passed
patriknw added a commit that referenced this issue Nov 23, 2020
* problem described in in issue
* temp (ask) ActorRef shouldn't be cached
* similar to to the ActorRef compression that was fixed
  in #28823
* temp ActorRef doesn't contain the ActorRef uid so it can
  be resolved to an ActorRef with an old cached Association
* additionally, invalidate the cached Association if it has
  been removed after quarantine
* test that reproduces the problem in the issue
* also verified with the Main example in the issue
patriknw added a commit that referenced this issue Dec 4, 2020
* problem described in in issue
* temp (ask) ActorRef shouldn't be cached
* similar to to the ActorRef compression that was fixed
  in #28823
* temp ActorRef doesn't contain the ActorRef uid so it can
  be resolved to an ActorRef with an old cached Association
* additionally, invalidate the cached Association if it has
  been removed after quarantine
* test that reproduces the problem in the issue
* also verified with the Main example in the issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants