allow java21 in jre matrix #12281
allow java21 in jre matrix #12281
Conversation
There was a problem hiding this comment.
If you silence a warning, it is forever lost. If we should be fixing a warning, we can silence it, but we should at least create a tracking issue for it so we can fix it. Adding transient for serializable problems is not a panacea. For most of those I'd much rather continue having broken serialization (e.g., by suppressing the warning) than acting as if serialization works when it is really broken.
| @@ -25,8 +25,8 @@ | |||
| */ | |||
| public class StatusException extends Exception { | |||
| private static final long serialVersionUID = -660954903976144640L; | |||
| private final Status status; | |||
| private final Metadata trailers; | |||
| private final transient Status status; | |||
There was a problem hiding this comment.
Ordinarily status will never be null. This allows it to start being null, which is just making new problems. Before making changes here we should look at #1913 . I'd be fine suppressing the warning for this class with a comment pointing to issue 1913.
There was a problem hiding this comment.
The real fix making these Exceptions serializable is a big task. So we should do @SuppressWarnings("serial") for now and open a tracking issue for this? Or I think there already exists #1913. I'll probable drop a comment there?
| @@ -365,14 +365,14 @@ private static int parseNanos(String value) throws ParseException { | |||
| private static long normalizedDuration(long seconds, int nanos) { | |||
| if (nanos <= -NANOS_PER_SECOND || nanos >= NANOS_PER_SECOND) { | |||
| seconds = checkedAdd(seconds, nanos / NANOS_PER_SECOND); | |||
| nanos %= NANOS_PER_SECOND; | |||
| nanos %= (int) NANOS_PER_SECOND; | |||
There was a problem hiding this comment.
Should NANOS_PER_SECOND be changed to an int? It looks like all usages are math with ints.
| @@ -34,7 +34,7 @@ public final class AnonymousInProcessSocketAddress extends SocketAddress { | |||
|
|
|||
| @Nullable | |||
| @GuardedBy("this") | |||
| private InProcessServer server; | |||
| private transient InProcessServer server; | |||
There was a problem hiding this comment.
Serializing this object won't work. I'd be tempted to make it fail explicitly.
| @@ -37,7 +37,7 @@ | |||
| public class GrpcServlet extends HttpServlet { | |||
| private static final long serialVersionUID = 1L; | |||
|
|
|||
| private final ServletAdapter servletAdapter; | |||
| private final transient ServletAdapter servletAdapter; | |||
There was a problem hiding this comment.
Are we able to make ServletAdapter serialable?
There was a problem hiding this comment.
We need to do cascade of serializations to achieve this. Interface ServerTransportListener one of the data member then needs to be serialized too. Then there are different implementations of this interface... Do we make changes to all of them?
There was a problem hiding this comment.
better suppressing the warning
There was a problem hiding this comment.
There's little-to-no hope of getting all of that serializable besides it being really invasive, so just suppressing the warning is best. We don't even need to open a bug in this case.
| @@ -220,6 +220,7 @@ public boolean shouldAccept(Runnable command) { | |||
| protected final Queue<LrsRpcCall> loadReportCalls = new ArrayDeque<>(); | |||
| protected final AtomicBoolean adsEnded = new AtomicBoolean(true); | |||
| protected final AtomicBoolean lrsEnded = new AtomicBoolean(true); | |||
| @SuppressWarnings("this-escape") | |||
There was a problem hiding this comment.
These initializations could be moved to @Before
| @@ -101,6 +101,7 @@ void writeFrame( | |||
| */ | |||
| private volatile boolean cancelled; | |||
|
|
|||
| @SuppressWarnings("this-escape") | |||
There was a problem hiding this comment.
Not a single this-escape was fixed. Are you thinking we should just disable the warning for the entire project? Otherwise, we might want to allow it on particular lines, which we feel are safe. You can do that via something like:
@SuppressWarnings("this-escape")
MessageFramer framer = new MessageFramer(this, bufferAllocator, statsTraceCtx);
this.framer = framer;(That won't work in all cases, but would work in most it looks like. It wouldn't work on setCumulator())
There was a problem hiding this comment.
Compiler is giving warnings even with this format. It is asking to suppress "this-escape" at constructor level.
There was a problem hiding this comment.
Disable the warning for the entire project?
There was a problem hiding this comment.
I'm now leaning toward keeping it enabled, as it looks like it generally tells us we are doing something wrong. I do agree the check is a bit overzealous, but it requires a public, non-final class. We shouldn't be doing that, as the API is quite hard to get right. But all of these are @Internal, and they are actually outliers. I think it may make sense to silence them here, so if someone adds a new instance of the warning it'll be more obvious. It's actually a pretty good tell-tale that the class should be package-private and/or final.
The one semi-exception to "it is @Internal" is MultiChildLoadBalancer. It is internal today, but is intended to be public in the future. And it should be fixed before it is used more widely.
There was a problem hiding this comment.
The one semi-exception to "it is
@Internal" is MultiChildLoadBalancer. It is internal today, but is intended to be public in the future. And it should be fixed before it is used more widely.
So we should open a tracking issue for this? Or is there any already pre-existing issue for making MultiChildLoadBalancer public then we may add a comment on that mentioning this? I couldn't find one.
No description provided.