Replace `new DefaultStreamMessage()` to `StreamMessage.streaming()` #4717

injae-kim · 2023-03-02T15:53:11Z

Related PR #4696, Issue #4253

Motivation:

   static <T> StreamMessageWriter<T> streaming() {
      return new DefaultStreamMessage<>();
So you want to deprecate this class in the follow-up PR?
I mean deprecating the whole class because it doesn't need to be exposed to public.

See Add StreamMessageWriter and StreamMessage.streaming() #4696 (comment)
We don't want to expose new DefaultStreamMessage() directly cause we have StreamMessage.streaming() instead

Modifications:

Replace new DefaultStreamMessage() to StreamMessage.streaming()

Result:

Now we can hide new DefaultStreamMessage() more and use StreamMessage.streaming() instead

ikhoon

Thanks for cleaning up!

injae-kim · 2023-03-03T03:32:09Z

armeria/core/src/main/java/com/linecorp/armeria/common/multipart/MultipartEncoder.java

Lines 153 to 161 in ff25d6f

    
           private static DefaultStreamMessage<StreamMessage<HttpData>> newEmitter(Subscription upstream) { 
        
               final DefaultStreamMessage<StreamMessage<HttpData>> emitter = 
        
                       new DefaultStreamMessage<StreamMessage<HttpData>>() { 
        
                           @Override 
        
                           protected void onRequest(long n) { 
        
                               // A BodyPart is converted to a StreamMessage<HttpData> one to one. 
        
                               upstream.request(n); 
        
                           } 
        
                       };

FYI, I found that some classes extends or overrides DefaultStreamMessage directly on another module.
So If we make DefaultStreamMessage package-private, we can't overrides like above on MultipartEncoder.

Also, If we add @Deprecated annotation on DefaultStreamMessage, it's looks weird on my IDE. (using DefaultStreamMessage directly looks like above, deprecated)

So I just replace new DefaultStreamMessage() to StreamMessage.streaming() as much as possible in this PR~!

trustin · 2023-03-03T07:20:19Z

So I just replace new DefaultStreamMessage() to StreamMessage.streaming() as much as possible in this PR~!

Sounds good to me.

trustin

Thanks a lot, @injae-kim !

minwoox · 2023-03-06T05:22:27Z

How about at least adding @Deprecated to the constructor so that people use the static method instead?

/**
 * Creates a new instance.
 * 
 * @deprecated Use ...
 */
@Deprecated
public DefaultStreamMessage() {...}

injae-kim · 2023-03-06T08:17:36Z

If we add @Deprecated annotation on DefaultStreamMessage constructor, DefaultHttpResponse, DefaultHttpRequest also looks deprecated cause they extend DefaultStreamMessage..

@minwoox , is it okay? then I'll add @Deprecated and update this PR~!

minwoox · 2023-03-06T08:34:40Z

@minwoox , is it okay? then I'll add @deprecated and update this PR~!

I think it's okay and we should so that users no more use the constructor of DefaultStreamMessage. If we don't, users will still use the constructor and it will bother users when we hit 2.0 and remove the constructor.

DefaultHttpResponse, DefaultHttpRequest

I realized that we also need to deprecate those APIs. Users should use HttpResponse.streaming() and HttpRequest.streaming(headers) instead. Could you also deprecate those?

injae-kim · 2023-03-08T08:49:45Z

📌 ~~I'll resolve the conflict soon~~ Done.

minwoox · 2023-03-08T10:35:16Z

core/src/main/java/com/linecorp/armeria/common/stream/AbstractStreamMessageWriter.java

@@ -26,7 +26,7 @@
 import com.linecorp.armeria.internal.common.stream.AbortingSubscriber;
 import com.linecorp.armeria.internal.common.stream.StreamMessageUtil;

-abstract class AbstractStreamMessageWriter<T> extends CancellableStreamMessage<T> implements StreamWriter<T> {
+abstract class AbstractStreamWriter<T> extends CancellableStreamMessage<T> implements StreamWriter<T> {


Oops, it seems like the file name isn't changed. 😆

armeria/core/src/main/java/com/linecorp/armeria/common/stream/AbstractStreamMessageWriter.java

Line 29 in e7b02d4

abstract class AbstractStreamMessageWriter<T> extends CancellableStreamMessage<T> implements StreamWriter<T> {

oh on latest master branch, still AbstractStreamMessageWriter .

It's better to rename AbstractStreamMessageWriter -> AbstractStreamWriter?
(cause we rename StreamMessageWrtier -> StreamWriter)

or.. I don't rename AbstractStreamMessageWriter in previous PR cause it implements CancellableStreamMessage + StreamWriter 😅

minwoox · 2023-03-08T10:44:48Z

kotlin/src/main/kotlin/com/linecorp/armeria/internal/common/kotlin/FlowCollectingPublisher.kt

+                // Delegate to coroutine's exception handling mechanism,
+                // which ignores CancellationException.


Suggested change

// Delegate to coroutine's exception handling mechanism,

// which ignores CancellationException.

We don't delegate anymore so let's just remove this comment. 😄

✅ updated!

minwoox

Thanks a lot, @injae-kim for cleaning this up!

jrhee17

Thanks @injae-kim ! 🙇 👍 🙇

ikhoon · 2023-03-09T07:44:24Z

core/src/main/java/com/linecorp/armeria/internal/common/DefaultHttpRequest.java

+ * @deprecated Use {@link HttpRequest#streaming(RequestHeaders)} instead.
 */
+@Deprecated


It is in internal. Do we need @Deprecated?

Oops, no we don't need it. Let me revert it. 😓

thanks for catching 🙇

ikhoon · 2023-03-09T09:20:03Z

Thanks for cleaning up! 🚀🚀

injae-kim requested review from ikhoon, jrhee17, minwoox and trustin as code owners March 2, 2023 15:53

injae-kim force-pushed the remove-default-stream-message-usage branch 2 times, most recently from f93df5f to c40543e Compare March 2, 2023 16:24

ikhoon approved these changes Mar 3, 2023

View reviewed changes

ikhoon added the cleanup label Mar 3, 2023

ikhoon added this to the 1.23.0 milestone Mar 3, 2023

injae-kim mentioned this pull request Mar 3, 2023

Add StreamMessageWriter and StreamMessage.streaming() #4696

Merged

trustin approved these changes Mar 3, 2023

View reviewed changes

injae-kim mentioned this pull request Mar 3, 2023

Remove StreamMessageWriter and replace it to StreamWriter #4718

Merged

injae-kim force-pushed the remove-default-stream-message-usage branch from 0717db4 to bffad77 Compare March 6, 2023 13:39

injae-kim added 2 commits March 8, 2023 19:24

Replace new DefaultStreamMessage() to StreamMessage.streaming()

1fae4b7

Deprecate DefaultStreamMessage constructor and sub types

d6ad37a

injae-kim force-pushed the remove-default-stream-message-usage branch from bffad77 to ffff582 Compare March 8, 2023 10:26

Resolve conflict with latest master

a378fa7

injae-kim force-pushed the remove-default-stream-message-usage branch from ffff582 to a378fa7 Compare March 8, 2023 10:33

minwoox reviewed Mar 8, 2023

View reviewed changes

Rename AbstractStreamMessageWriter file to AbstractStreamWriter

736c80d

minwoox reviewed Mar 8, 2023

View reviewed changes

minwoox approved these changes Mar 8, 2023

View reviewed changes

Address @minwoox comment

da0bb9b

jrhee17 approved these changes Mar 8, 2023

View reviewed changes

ikhoon reviewed Mar 9, 2023

View reviewed changes

Remove deprecation to internal classes

b755c78

ikhoon merged commit 179d9a5 into line:master Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `new DefaultStreamMessage()` to `StreamMessage.streaming()` #4717

Replace `new DefaultStreamMessage()` to `StreamMessage.streaming()` #4717

injae-kim commented Mar 2, 2023

ikhoon left a comment

injae-kim commented Mar 3, 2023 •

edited

Loading

trustin commented Mar 3, 2023

trustin left a comment

minwoox commented Mar 6, 2023

injae-kim commented Mar 6, 2023

minwoox commented Mar 6, 2023 •

edited

Loading

injae-kim commented Mar 8, 2023 •

edited

Loading

minwoox Mar 8, 2023

injae-kim Mar 8, 2023

injae-kim Mar 8, 2023

minwoox Mar 8, 2023

injae-kim Mar 8, 2023

minwoox left a comment

jrhee17 left a comment

ikhoon Mar 9, 2023

minwoox Mar 9, 2023

injae-kim Mar 9, 2023

ikhoon commented Mar 9, 2023

		// Delegate to coroutine's exception handling mechanism,
		// which ignores CancellationException.

Replace new DefaultStreamMessage() to StreamMessage.streaming() #4717

Replace new DefaultStreamMessage() to StreamMessage.streaming() #4717

Conversation

injae-kim commented Mar 2, 2023

Motivation:

Modifications:

Result:

ikhoon left a comment

Choose a reason for hiding this comment

injae-kim commented Mar 3, 2023 • edited Loading

trustin commented Mar 3, 2023

trustin left a comment

Choose a reason for hiding this comment

minwoox commented Mar 6, 2023

injae-kim commented Mar 6, 2023

minwoox commented Mar 6, 2023 • edited Loading

injae-kim commented Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Mar 9, 2023

Replace `new DefaultStreamMessage()` to `StreamMessage.streaming()` #4717

Replace `new DefaultStreamMessage()` to `StreamMessage.streaming()` #4717

injae-kim commented Mar 3, 2023 •

edited

Loading

minwoox commented Mar 6, 2023 •

edited

Loading

injae-kim commented Mar 8, 2023 •

edited

Loading