Skip to content

Conversation

@artech-git
Copy link
Contributor

@artech-git artech-git commented Apr 12, 2023

Description

In My understanding using lazy_static with some subtle ways to implement a feature is not very idiomatic to write code
To eliminate the Usage for Lazy_static on runtime initialization, which can be directly accomplished through once_cell
and makes our implementation more clear.

Though there are multiple crates available to perform global runtime initialization method, but using the one already in the project (which is once_cell) is suitable for this improvement.

lazy_static! {
  pub static ref SOME_VALUE: Arc<Mutex<i32>> = Arc::new(Mutex::new());
}

replaced with the following method

  pub static SOME_VALUE: Lazy<Arc<Mutex<i32>>> = Lazy::new( || Arc::new(Mutex::new()));

since the Lazy<T,F> implements Deref & DerefMut traits we can directly call the methods for the types present under it

but for some types which require a std::fmt::Debug implementation and also have some standalone implementation for it

lazy_static! {
  #[derive(Debug)]
  pub static ref SOME_VALUE: Arc<Mutex<i32>> = Arc::new(Mutex::new());
}

 impl SOME_VALUE { 
 .
 .
}

what I refactored to

  pub static SOME_VALUE: Lazy<InnerType> = Lazy::new( || Arc::new(Mutex::new()));

  struct InnerType(Arc<Mutex<i32>>);

  impl Deref for InnerType { 
      type Target = Arc<Mutex<i32>>;
      fn deref(&self) -> &Self::Target {
          &self.0
      }
    }
  impl DerefMut for InnerType { 
      fn deref_mut(&mut self) -> &mut Self::Target {
          &mut self.0
      }
  }
 
 impl Debug for CachedFilesInnerType {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        f.write_str("InnerType { __private_field: () }")
    }
 }
 
  impl InnerType {
  .
  .
  .
  }

I first concealed the type into a child struct and implemented Deref and DerefMut on it so that we call methods directly
and since Lazy type already implements Deref traits so it directly call the appropiate method without any problem through out the rest of the code

If you find something missing or these changes won't be according to your standards or just anything which is necessary and seems missing in my knowledge do let me know 😊 I will be more than happy to revise my work.


It will nice if someone can please test and verify the below marked parameters to ensure these changes won't cause any undefined behavior

This PR has:

  • been tested to ensure log ingestion and log query works.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

1.utilizing Lazy type for init wherever possible

2.for more types where static types demand some implementation a child
struct is created to accomodate the values of the inner and type and
traits Deref, DerefMut & Debug are explicitly implemented
@nitisht nitisht requested a review from trueleo April 12, 2023 12:24
@nitisht
Copy link
Member

nitisht commented Apr 12, 2023

Thanks @artech-git . cc @trueleo can you take a look

@trueleo
Copy link
Contributor

trueleo commented Apr 12, 2023

@artech-git

since the Lazy<T,F> implements Deref & DerefMut traits we can directly call the methods for the types present under it
but for some types which require a std::fmt::Debug implementation and also have some standalone implementation for it

We already have derive_more in the dependencies which provides derive macros for Deref and DerefMut, I made some changes to your branch and pushed a PR. artech-git#1 , Accept these changes and i think this is good to merge.

Copy link
Contributor

@trueleo trueleo 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

@artech-git
Copy link
Contributor Author

Thanks @trueleo for bringing that to my knowledge 😊, also there was one smoke test failing for docker-compose can you educate me what that is about ?

@nitisht
Copy link
Member

nitisht commented Apr 12, 2023

Thanks @trueleo for bringing that to my knowledge 😊, also there was one smoke test failing for docker-compose can you educate me what that is about ?

Hey @artech-git this is a Docker compose based test which is a little flaky sometimes. We need to fix the CI. I tested the PR locally, seems to be working fine. Merging this now

@nitisht nitisht merged commit f4ad0ad into parseablehq:main Apr 12, 2023
@artech-git
Copy link
Contributor Author

Thanks @trueleo for bringing that to my knowledge 😊, also there was one smoke test failing for docker-compose can you educate me what that is about ?

Hey @artech-git this is a Docker compose based test which is a little flaky sometimes. We need to fix the CI. I tested the PR locally, seems to be working fine. Merging this now

Thanks for the info

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.

3 participants