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())
No description provided.