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

Deprecate Stream-related API #1195

Merged
merged 9 commits into from
Sep 14, 2023
Merged

Deprecate Stream-related API #1195

merged 9 commits into from
Sep 14, 2023

Conversation

jyemin
Copy link
Collaborator

@jyemin jyemin commented Sep 11, 2023

Adds:

  • TransportSettings abstract class
  • NettyTransportSettings subclass of TransportSettings
  • A new method on MongoClientSettings to configure TransportSettings

Deprecates:

  • MongoClientSettings#streamFactoryFactory
  • NettyStreamFactoryFactory
  • NettyStreamFactory
  • AsynchronousSocketChannelStreamFactory
  • AsynchronousSocketChannelStreamFactoryFactory
  • BufferProvider
  • SocketStreamFactory
  • Stream
  • StreamFactory
  • StreamFactoryFactory
  • TlsChannelStreamFactoryFactory

JAVA-5051

Copy link
Collaborator Author

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

Couple of things:

Though JEP 211 indicates that it should be otherwise, and my testing directly with javac confirms the expected behavior, when building the driver I'm getting warnings about imports of classes/interfaces that have been deprecated. I'm not sure why, or what to do about it. Options:

  1. Fully qualify all references to deprecated types so that the imports can be removed
  2. Disable linting for deprecation
  3. Live with the warnings until it's time to remove the deprecated types for 5.0

Scala test The_scala_package_should_mirror_parts_of_com.mongodb.connection_in_org.mongdb.scala.connection is passing by adding exceptions for the two new classes. Not sure whether to add Scala wrappers for them instead.

*/
@Sealed
@Immutable
public abstract class TransportSettings {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I still like this name... at least better than IoSettings. Note that Netty refers to its implementation as "Transport Services".

An alternative that keeps the same spirit would be something like TransportImplementationSettings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Going once...

Copy link
Member

Choose a reason for hiding this comment

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

I am good with the name.

@jyemin jyemin requested review from stIncMale and rozza September 11, 2023 23:13
Adds:

* TransportSettings abstract class
* NettyTransportSettings subclass of TransportSettings
* A new method on MongoClientSettings to configure TransportSettings

Deprecates:

* MongoClientSettings#streamFactoryFactory
* NettyStreamFactoryFactory
* NettyStreamFactory
* AsynchronousSocketChannelStreamFactory
* AsynchronousSocketChannelStreamFactoryFactory
* BufferProvider
* SocketStreamFactory
* Stream
* StreamFactory
* StreamFactoryFactory
* TlsChannelStreamFactoryFactory

JAVA-5051
jyemin and others added 3 commits September 12, 2023 18:11

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Valentin Kovalenko <valentin.male.kovalenko@gmail.com>
@jyemin jyemin requested a review from stIncMale September 12, 2023 23:39
@stIncMale
Copy link
Member

When I run ./gradlew clean check jar scalaCheck kotlinCheck testClasses docs -x test -x integrationTest -x spotlessApply with javac 17.0.3, I am still getting lots of warnings. Here is an example:

Task :driver-core:compileJava
/Users/valentin.kovalenko/Documents/projects/mongo-java-driver/driver-core/src/main/com/mongodb/MongoClientSettings.java:29: warning: [deprecation] StreamFactoryFactory in com.mongodb.connection has been deprecated
import com.mongodb.connection.StreamFactoryFactory;

@jyemin
Copy link
Collaborator Author

jyemin commented Sep 13, 2023

I am still getting lots of warnings.

Yes, I mentioned that here, and gave several options for how to proceed.

@stIncMale
Copy link
Member

Yes, I mentioned that #1195 (review), and gave several options for how to proceed.

Sorry, I thought the added @SuppressWarnings("deprecation") were supposed to solve that. I like "2. Disable linting for deprecation combined with modified 3: live without the lint until it's time to remove the deprecated types for 5.0 (we'll move the program elements into the internal package, remove @Deprecated from them, enable the lint back again).

@jyemin
Copy link
Collaborator Author

jyemin commented Sep 13, 2023

I went with option 2 and disabled linting for deprecation

@stIncMale
Copy link
Member

stIncMale commented Sep 13, 2023

A few more suppressions are needed, as there are still some warnings (or, rather notes). Here is an example:

Task :driver-sync:compileJava
Note: /Users/valentin.kovalenko/Documents/projects/mongo-java-driver/driver-sync/src/main/com/mongodb/client/internal/MongoClientImpl.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

@stIncMale
Copy link
Member

No, I was wrong. AbstractUnifiedTest already specifies @SuppressWarnings("deprecation") at the class level, and the note is still there. I guess, it's supposed to be like that.

@jyemin
Copy link
Collaborator Author

jyemin commented Sep 13, 2023

I guess, it's supposed to be like that.

I'm mystified by the behavior of the compiler in this regard.

Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM - So whats next after this PR?

As internally we continue to use StreamFactoryFactory is there a ticket for the replacements? I'm interested in what the alternative will look like.

@jyemin
Copy link
Collaborator Author

jyemin commented Sep 14, 2023

So whats next after this PR?

Similar to what we did with the Operation layer in 3.x. We'll remove the two deprecated methods on MongoClientSettings, and then move all thee deprecated types to com.mongodb.internal.connection. After that we can see if we want to refactor anything. One obvious thing is to get rid of StreamFactoryFactory and just use StreamFactory directly. Another is to get rid of Stream#supportsAdditionalTimeout as that was added only due to Stream being a public type. We'll also need to deal with the removal of ServerAddress#getSocketAddress, which we can do once Stream is no longer public. There may be other simplifications, but these are the ones I'm aware of.

The point of deprecation/removal here is not that this is not a useful abstraction for the driver. It's that it doesn't need to be part of the driver API, as afaik no-one has ever needed to create their own implementation of StreamFactoryFactory and having these types as as part of the API makes it hard to refactor and evolve the driver.

@jyemin jyemin merged commit 2ed89c9 into mongodb:master Sep 14, 2023
@jyemin jyemin deleted the j5051 branch September 14, 2023 13: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.

None yet

3 participants