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

Change materializer type for chunkUploadSink in S3 DSL's #280

Merged

Conversation

mdedetrich
Copy link
Contributor

In #279 I forgot to update the public S3 DSL's

@mdedetrich
Copy link
Contributor Author

@pjfanning So this is the PR that actually changes public signatures and it did trigger a MiMa incompatiblity check but it can be a false positive, i.e. see https://github.com/lightbend/mima#incompatiblesignatureproblem which I believe is the case.

I can double check this by writing a program that is compiled against Sink[(UploadPartResponse, immutable.Iterable[C]), NotUsed] and then at runtime loading a pekko-connectors-s3 jar that is compiled with Sink[(UploadPartResponse, immutable.Iterable[C]), _] to see if it fails at runtime

@pjfanning
Copy link
Contributor

We can add the 20 binary compatibility exceptions. I understand that the Mima Issues can often be items that don't affect users in real world scenarios. It is worth writing an external test to find out if we are stuck having to consider this as a v1.1.0 change.

@mdedetrich mdedetrich force-pushed the change-chunk-upload-sink-mat-param-any-dsl branch from 575983c to 8f63673 Compare November 13, 2023 09:05
@mdedetrich
Copy link
Contributor Author

mdedetrich commented Nov 13, 2023

@pjfanning So I can indeed confirm that the warning from MiMa in this case is a false positive and this PR doesn't actually break any binary compatiblity.

The easiest way to confirm this is to hot patch the s3 tests at runtime so that rather than using the compiled classes from s3 project it uses a locally deployed S3 that has the change in this PR, to do this

  • Checkout this PR's branch and run +s3/publishLocal

  • Checkout main

  • Modify the s3 project in build.sbt so it looks like this

    lazy val s3 = pekkoConnectorProject("s3", "aws.s3", Dependencies.S3,
    MetaInfLicenseNoticeCopy.s3Settings)
    .settings(
      Test / fullClasspath := {
        val current = (Test / fullClasspath).value
        current.map {
          case f if f.data.getAbsolutePath.contains("s3/target/scala-2.13/classes") =>
            Attributed[File](
              file(
                "/Users/mdedetrich/.ivy2/local/org.apache.pekko/pekko-connectors-s3_2.13/1.0.1+9-575983c5-SNAPSHOT/jars/pekko-connectors-s3_2.13.jar"))(
              AttributeMap.empty)
          case rest => rest
        }
      })

    Make sure to change the paths where neccessary, i.e. the local path to where you just published S3.

  • Run the tests with s3/test

What this change does it only modifies the test classpath which means that when you do s3/test/compile it will generate classes without the change in this branch (since you should be checked out in main) but when you run the tests with s3/test it will use the jar with the change in this branch which you just published locally (note that compiled test code, i.e. the tests written in scala-test sits in test-classes so its unchanged by this hotpatch)

You can play around with show s3/Test/fullClasspath to confirm this.

The PR has been updated to add the various MiMa filters.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

Lgtm

@mdedetrich mdedetrich merged commit 32facdd into apache:main Nov 13, 2023
49 of 50 checks passed
@mdedetrich mdedetrich deleted the change-chunk-upload-sink-mat-param-any-dsl branch November 13, 2023 14:57
@pjfanning pjfanning added this to the 1.0.2 milestone Dec 28, 2023
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.

None yet

2 participants