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

Added xreadgroup, xtrim, xclaim, xautoclaim, xpending commands #52

Merged

Conversation

Swoorup
Copy link
Contributor

@Swoorup Swoorup commented Jun 10, 2022

  • Adds xreadgroup (mostly same as xread), xtrim commands.
  • Updates xgroupcreate such that it allows to specify creation of stream

@ChristopherDavenport
Copy link
Collaborator

Is mima off? switching to defaults args I didn't think was binary compatible.

@Swoorup
Copy link
Contributor Author

Swoorup commented Jun 11, 2022

Is mima off? switching to defaults args I didn't think was binary compatible.

In build.sbt, the mimaPreviousArtifacts is empty and has comment // Bincompat breaking till next release.

Looks like following will fix the incompatability. Any thoughts? @ChristopherDavenport

  def xgroupcreate[F[_]: RedisCtx](stream: String, groupName: String, startId: String, mkStream: Boolean): F[Status] = {
    val mkStreamFragment = Alternative[List].guard(mkStream).as("MKSTREAM")
    RedisCtx[F].unkeyed(NEL("XGROUP", "CREATE" :: stream :: groupName :: startId :: mkStreamFragment))
  }

  def xgroupcreate[F[_]: RedisCtx](stream: String, groupName: String, startId: String): F[Status] = 
    xgroupcreate(stream, groupName, startId, false)

@ChristopherDavenport
Copy link
Collaborator

ChristopherDavenport commented Jun 11, 2022

If you make the current one package private that will work great.

The one you delegate to the new one in.

@Swoorup
Copy link
Contributor Author

Swoorup commented Jun 11, 2022

If you make the current one package private that will work great.

The one you delegate to the new one in.

Hmm, it still complains of

 filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("io.chrisdavenport.rediculous.RedisCommands.xgroupcreate")
[error] rediculous: Failed binary compatibility check against io.chrisdavenport:rediculous_2.13:0.3.2! Found 1 potential problems
[error]  * static method xgroupcreate(java.lang.String,java.lang.String,java.lang.String,io.chrisdavenport.rediculous.RedisCtx)java.lang.Object in class io.chrisdavenport.rediculous.RedisCommands does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("io.chrisdavenport.rediculous.RedisCommands.xgroupcreate")

using private[rediculous] def xgroupcreate[F[_]: RedisCtx](stream: String, groupName: String, startId: String): F[Status]

@ChristopherDavenport
Copy link
Collaborator

Fairly certain that's normal. Feel free to add the exclusion. Will confirm with @armanbilge just to be double safe.

@armanbilge
Copy link
Contributor

My recommendation would be to leave that method public but deprecated, like so:

@deprecated("x.y.z.", "use ... instead")
def xgroupcreate[F[_]](stream: String, groupName: String, startId: String, redisCtx: RedisCtx[F]): F[Status]

In theory, static methods are not really a concern since they would only be used by hypothetical Java interop. But the bigger problem is that every time you add a filter, it might mask real binary-incompatible changes in the future. Unfortunately MiMa filters are just not fine grained enough, for example to distinguish between static and non-static methods.

@Swoorup
Copy link
Contributor Author

Swoorup commented Jun 11, 2022

Done.

@ChristopherDavenport
Copy link
Collaborator

How is adding a value to a sealed trait safe? Or is that just source breaking not mima breaking?

My brain is trying to put the pieces together here.

Comment on lines +267 to +269
case class LastConsumed(stream: String) extends StreamOffset {
override def offset: String = ">"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@ChristopherDavenport you mean this change? You are right, this is not backwards-compatible. See:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is I gather, only if pattern matching on the user side? Which seems a rather odd use in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChristopherDavenport Anything changes required on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the best way is to just release a new minor. Its limited changes and as you mentioned no one is probably doing this so the only burden is on intermediate libraries.

Are there any other values here we should add?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Answering my own question this looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be all, I believe.

@Swoorup
Copy link
Contributor Author

Swoorup commented Jun 17, 2022

Added xpendingsummary, xclaimsummary, xclaimdetail, xautoclaimsummary, xautoclaimdetail.

@Swoorup Swoorup changed the title Added xreadgroup, xtrim commands Added xreadgroup, xtrim, xclaim, xpending commands Jun 17, 2022
@Swoorup Swoorup changed the title Added xreadgroup, xtrim, xclaim, xpending commands Added xreadgroup, xtrim, xclaim, xautoclaim, xpending commands Jun 18, 2022
@ChristopherDavenport ChristopherDavenport merged commit 3aacc67 into davenverse:main Jul 8, 2022
@Swoorup Swoorup deleted the sj-add-more-stream-cmds branch July 8, 2022 02:59
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