Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
  • Oct 17 12:07
    gafiatulin synchronize #52
  • Oct 15 16:55

    matthewgraf on master

    S3Store: block on blocking ec Merge pull request #49 from gaf… (compare)

  • Oct 15 16:55
    matthewgraf closed #49
  • Oct 10 00:15
    gafiatulin synchronize #49
  • Oct 08 20:32

    matthewgraf on master

    use openjdk8 instead of oraclej… Merge pull request #55 from len… (compare)

  • Oct 08 20:32
    matthewgraf closed #55
  • Oct 08 20:22
    matthewgraf opened #55
  • Oct 08 20:22

    matthewgraf on openjdk8

    use openjdk8 instead of oraclej… (compare)

  • Oct 08 20:14
    matthewgraf synchronize #49
  • Oct 08 19:51

    matthewgraf on master

    ConcurrentEffect, Effect -> Con… Merge pull request #48 from gaf… (compare)

  • Oct 08 19:51
    matthewgraf closed #48
  • Oct 04 12:24
    gafiatulin commented #54
  • Oct 04 12:24
    gafiatulin edited #54
  • Oct 04 12:23
    gafiatulin synchronize #54
  • Oct 04 11:09
    gafiatulin synchronize #54
  • Sep 23 11:52
    gafiatulin commented #54
  • Sep 23 10:19
    gafiatulin synchronize #54
  • Sep 23 10:18
    gafiatulin edited #54
  • Sep 23 10:18
    gafiatulin synchronize #54
  • Sep 23 10:16
    gafiatulin synchronize #54
Rolando Manrique
@rolandomanrique
Resource would probably be better, we started this with cats 0.x before Resource so there are likely improvements to be done
Doug Clinton
@DougC
@rolandomanrique thanks
matthewgraf
@matthewgraf

Hey all. I'm migrating a project to blobstore 0.4.1, which also involved migrating our project from fs2 0.x to 1.x.

I notice bufferedPut now requires a ConcurrentEffect and ContextShift as implicit parameters. I'm trying to understand how these are actually used by bufferedPut or a method that it calls. Would anyone be able to explain?

Rolando Manrique
@rolandomanrique
Hi Matt, I don’t have the code in front of me, but cats 1.x introduced ContextShift as a mechanism to wrap a thread pool and allows to run concurrent ops... if I remember correctly bufferedPut would write to disk and then upload to store and it does it with some concurrent streams which would require those constructs
matthewgraf
@matthewgraf
Got it. That makes sense for the most part. I just don't entirely understand how the ContextShift is used in this particular method. I see it as part of the generic type but thats about it. I'm probably missing something obvious here, its been a while since I looked at Scala code
this generic syntax in particular is throwing me off
protected[blobstore] def bufferToDisk[F[_] : ConcurrentEffect : ContextShift]
Rolando Manrique
@rolandomanrique
That is just sintaxis sugar... it’s the same as having implicit parameters implicit F: ConcurrentEffect[F], S:ContextShift[F]
matthewgraf
@matthewgraf
Oooh thanks. I was pretty confused about that
Rolando Manrique
@rolandomanrique
To set up implicit context shift, timer, and clock you can just use IOApp in your Main class... this will use default execution context, if you need to customize thread pools it’s a bit trickier but can be done
matthewgraf
@matthewgraf
@rolandomanrique have you had success importing cats.effect.IOApp? No matter where I have the import statement, the compiler cannot detect the implicit value contextShift provided by IOApp
matthewgraf
@matthewgraf
hm.. I might just forget about using IOApp. We already have our own ExecutionContext which we could use
matthewgraf
@matthewgraf
@rolandomanrique I think we might want to specify the expected behavior of certain operations e.g. remove. I notice S3Store.remove will succeed if the target file does not exist, whereas FileStore will fail in this case
I'd be in favor of having remove(path) succeed when the path does not exist. I could make an update to FileStore and whatever other stores are not consistent with this behavior
Curious to hear other people's thoughts as well
Rolando Manrique
@rolandomanrique
I’m all for consistency, and sound like it should not fail if file is missing since the end state is the desired (file is not there)
matthewgraf
@matthewgraf
Yeah, I was thinking the same thing
matthewgraf
@matthewgraf
Victor
@gafiatulin
Hi! Can someone take a look at PR I submitted?
Rolando Manrique
@rolandomanrique
@gafiatulin thanks for the PR... great to see more store impls coming... on a quick pass it looks good to me, I’ll reach out to Mission Lane (formerly LendUp) to review, merge and release
matthewgraf
@matthewgraf
@gafiatulin sweet. I should have some time today to look over it
matthewgraf
@matthewgraf
@gafiatulin sorry for the delay. Looks good. I had 1 minor question which isn't really a blocker but just a possible optimization
matthewgraf
@matthewgraf
once I hear back I can go ahead and get that merged and deploy a new version. Possibly this weekend if I hear back from you
matthewgraf
@matthewgraf
@gafiatulin I'm about to release version 0.5.0 which includes your changes
Jostein Gogstad
@jgogstad
would have loved to have #47 in that release, Path currently behaves unpredicably (from a client’s perspective) with the different schemes
do you have time to look over that one, and the proposed changes to the SFTP store? (#49, #50, #52), using least powerful effect types would also be nice, #48
matthewgraf
@matthewgraf
Ah, yes I might have some time today to start looking through these. @rolandomanrique it would be great to get a second set of eyes on these as well
Great to see these contributions! I haven't been watching the repo too closely so feel free to ping here if I haven't acknowledged a PR yet
Rolando Manrique
@rolandomanrique
Sure thing... thanks, I’ll take a look soon
Rolando Manrique
@rolandomanrique
@matthewgraf @gafiatulin these all look good to me, the last "opinionated" of making all stores regular classes (as opposed to case classes) to hide constructor parameters makes sense to me. Regarding CS.evalOn changes I'm less familiar with it but assuming this makes it explicitly run in the provided EC which sounds safer to me. Maybe @Daenyth @Igosuki @DougC have a stronger position on any of these changes since they have worked more on the upgrades to newer scala/cats/fs2 versions, but otherwise I'm good with these changes.
Jostein Gogstad
@jgogstad
that’s correct. It’s a bit surprising as a client when I pass in an ExecutionContext that is bound to a variable named blockingExecutionContext and then blocking IO is done on whatever context the current thread is executing on
Gavin Bisesi
@Daenyth
I recommend updating to cats-effect 1.4 at least (if you're not on 2.x on master already) and using the Blocker interface, it adds a lot of clarity
it's absolutely necessary for healthy thread pool usage to use a blocking EC correctly; the recommended cats-effect setup (and the way that the ecosystem works) is that you use a fixed-size (= cores) thread pool as your primary EC that backs your ContextShift, and a cached thread pool for jvm-thread-blocking calls, using evalOn to make sure those calls happen on the thread blocking pool
if you do thread-blocking calls without using evalOn (or related) to put it on the right pool, you risk severely hampering the application's efficiency
Victor
@gafiatulin
@/all PR for cross-compilation to 2.13: #54
Jostein Gogstad
@jgogstad
hey guys, any progress on this?
If you would like more maintainers, @gafiatulin and I volunteer
Rolando Manrique
@rolandomanrique
Hi @jgogstad sorry for the delay, its been hard to sync up since Matt is currently the only one with merge access and access to LendUp's GPG keys... I'll try to sync up with him and get this merged and published ASAP. We will also be moving this to a separate GitHub org so we can open it to more maintainers. Once we merge and publish these last PRs we will go ahead and do that.
matthewgraf
@matthewgraf
Starting to merge these, running into a build error. Looks like we need to use openjdk8 instead or oraclejdk8
ah. looks like an easy fix
matthewgraf
@matthewgraf
I got 48 merged, and the master build fixed. I gotta get some other work done atm, but will revisit later this evening/tonight
Sorry again for the delay. Ditto what Rolando said about moving this to another org, we need more maintainers besides me :sweat:
Jostein Gogstad
@jgogstad
no problem, thanks for laying down the effort!
matthewgraf
@matthewgraf
No prob! Now that the build is fixed I should be able to get those other PRs merged today
matthewgraf
@matthewgraf

@gafiatulin would you mind pulling master into your branch? lendup/fs2-blobstore#49

Looks like Travis is using the travis.yml from your branch, not master. I thought it would merge master first and use travis.yml from there. Just trying to get a green build before merging

Victor
@gafiatulin
@matthewgraf #49 is green now
matthewgraf
@matthewgraf
Ok thank you. I'll get to it today
matthewgraf
@matthewgraf
@jgogstad can you fix conflicts on lendup/fs2-blobstore#50 ? I will merge this today. Trying to get this one in and #52 before doing a release
also @gafiatulin for lendup/fs2-blobstore#52
they might be straightforward but you probably have more context
Victor
@gafiatulin
@matthewgraf I fixed #52. It also includes changes from #50