Skip to content

Conversation

@bbalser
Copy link
Collaborator

@bbalser bbalser commented Mar 30, 2025

No description provided.

shutdown: triggered::Listener,
) -> futures::future::LocalBoxFuture<'static, anyhow::Result<()>> {
Box::pin(self.run(shutdown))
// let handle = tokio::spawn(async move { self.run(shutdown).await });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@michaeldjeffrey I am hoping to actually spawn this, but when i do I get a weird lifetime issue that I haven't figured out yet, maybe you got some ideas?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed the lifetime issues here c68a9fb, not sure if it is the best way, but it seems to work

@bbalser bbalser marked this pull request as ready for review March 31, 2025 20:31
AV: AuthorizationVerifier + Clone,
EV: EntityVerifier + Clone + Send + Sync + 'static,
AV: AuthorizationVerifier,
EV: EntityVerifier + Send + Sync + 'static,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to move Send + Sync + 'static to the definition of EntityVerifier as it will be required anywhere we use it?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably more concise: EntityVerifier: Send + Sync + 'static { trait functions }

Copy link
Contributor

Choose a reason for hiding this comment

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

also looks like we're already doing this in the trait definition about the AuthorizationVerifier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed d326a72

Cargo.toml Outdated
Comment on lines 139 to 140
[patch.'https://github.com/helium/proto']
helium-proto = { git = "https://www.github.com/helium/proto.git", branch = "jg/disco-shares-v2" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to leave this as a comment when we move back to main branch. It probably won't be the last time we need to override a proto branch of a dep.

#[async_trait]
pub trait AuthorizationVerifier: Send + Sync + 'static {
type Error;
type Error: std::error::Error + Send + Sync + 'static;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are always ClientError can we remove the associated type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed a223808

AV: AuthorizationVerifier + Clone,
EV: EntityVerifier + Clone + Send + Sync + 'static,
AV: AuthorizationVerifier,
EV: EntityVerifier + Send + Sync + 'static,
Copy link
Contributor

Choose a reason for hiding this comment

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

also looks like we're already doing this in the trait definition about the AuthorizationVerifier

@bbalser bbalser merged commit f642d86 into main Apr 2, 2025
27 checks passed
@bbalser bbalser deleted the bbalser/subscriber-activity branch April 2, 2025 17:47
macpie pushed a commit that referenced this pull request Apr 14, 2025
…orts (#981)

* Add receiving subscriber mapper activity requests to mobile ingestor

* update mobile verifier to process subscriber mapper activity messages

* update rewarder to use new subscriber mapping activity

* making saving actvity idempotent and fix tests

* remove unused subscriber_location and subscriber_verified mapping from mobile verifier

* remove unused tests

* Change to spawn tokio task and fix lifetime issues

* remove commented out code

* refactoring of subscriber mapping activity processing

* move send/sync to trait definition

* remove associated type for AuthorizationVerifier and EntityVerifier

* update proto dep back to master

* update beacon
macpie pushed a commit that referenced this pull request Apr 14, 2025
…orts (#981)

* Add receiving subscriber mapper activity requests to mobile ingestor

* update mobile verifier to process subscriber mapper activity messages

* update rewarder to use new subscriber mapping activity

* making saving actvity idempotent and fix tests

* remove unused subscriber_location and subscriber_verified mapping from mobile verifier

* remove unused tests

* Change to spawn tokio task and fix lifetime issues

* remove commented out code

* refactoring of subscriber mapping activity processing

* move send/sync to trait definition

* remove associated type for AuthorizationVerifier and EntityVerifier

* update proto dep back to master

* update beacon
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.

4 participants