8283779: Clarify API documentation of NetworkInterface with respect to configuration changes #20822

dfuch · 2024-09-02T13:48:55Z

Please find here a change that adds a few @apiNote and @implNote to NetworkInterface to clarify user expectation and implementation.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Change requires CSR request JDK-8339623 to be approved
Commit message must refer to an issue

Issues

JDK-8283779: Clarify API documentation of NetworkInterface with respect to configuration changes (Enhancement - P3)
JDK-8339623: Clarify API documentation of NetworkInterface with respect to configuration changes (CSR)

Reviewers

Mark Sheppard (@msheppar - Reviewer) Review applies to fe9c3a87
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20822/head:pull/20822
$ git checkout pull/20822

Update a local copy of the PR:
$ git checkout pull/20822
$ git pull https://git.openjdk.org/jdk.git pull/20822/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20822

View PR using the GUI difftool:
$ git pr show -t 20822

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20822.diff

Webrev

Link to Webrev Comment

dfuch · 2024-09-02T13:49:10Z

/issue JDK-8283779

bridgekeeper · 2024-09-02T13:49:32Z

👋 Welcome back dfuchs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-09-02T13:50:57Z

@dfuch This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8283779: Clarify API documentation of NetworkInterface with respect to configuration changes

Reviewed-by: alanb, msheppar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 78 new commits pushed to the master branch:

559fc71: 8339366: [jittester] Make it possible to generate tests without execution
6b5958d: 8339696: Clarify modeling scope of javax.lang.model.element
77468c2: 8339575: DumpingWithJavaAgent.java failed with missing expected output
86a2f9c: 8339644: Improve parsing of Day/Month in tzdata rules
a9bb043: 8339683: Simplify class data generation in InvokerBytecodeGenerator
7c0f013: 8339488: Extended NPE message doesn't handle CONSTANT_Dynamic
d53e405: 8339742: Refactor ClassFileImpl to allow loading Option classes lazily
c54fc08: 8338587: Internal XOF Methods for SHAKE128 and SHAKE256
88cccc1: 8339631: Fix block @jls and @JVMS tags
615a24f: 8338902: CDS flags are reported with wrong flag category
... and 68 more: https://git.openjdk.org/jdk/compare/ad40a122d632d65052b71125c0dfd58c54e3a521...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-09-02T13:51:31Z

@dfuch This issue is referenced in the PR title - it will now be updated.

openjdk · 2024-09-02T13:51:38Z

@dfuch The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-09-02T13:54:49Z

Webrevs

msheppar · 2024-09-02T15:28:53Z

Also, I think it would be useful and helpful to allude to the "snapshot" characteristics of a NetworkInterface instance in the opening description of the class

public final class NetworkInterface
extends Object
This class represents a Network Interface made up of a name, and a list of IP addresses assigned to this interface. It is used to identify the local interface on which a multicast group is joined. Interfaces are normally known by names such as "le0".

Add to this description, some explanation to the fact that an instance of a NetworkInterface is reflective of its configuration at the time it was created, and that it is possible for that configuration to change due to dynamic reconfiguration policies within a host environment i.e. your "snapshot" details.

These additions of implNote and apiNote to various methods will be helpful

AlanBateman · 2024-09-02T18:39:22Z

src/java.base/share/classes/java/net/NetworkInterface.java

+     * @implNote
+     * The returned array contains all or a subset of the InetAddresses that were
+     * bound to the interface at the time the {@linkplain #getNetworkInterfaces()
+     * interface configuration was read}


I think you have a typo here in that this method returns an Enumeration rather than an array.

AlanBateman · 2024-09-02T18:47:55Z

src/java.base/share/classes/java/net/NetworkInterface.java

+     * @apiNote
+     * The returned interface instance may reflect a snapshot of the
+     * configuration taken at the time the instance is created.
+     * To see updates to the configuration may, or may not, require


The sentence "To see updates ..." is hard to read. How about "The network configuration may change at any time, this method may need to be invoked to obtain a more up to date view of the network interface".

src/java.base/share/classes/java/net/NetworkInterface.java

Co-authored-by: Daniel Jelinski <djelinski1@gmail.com>

src/java.base/share/classes/java/net/NetworkInterface.java

dfuch · 2024-09-03T13:25:34Z

/csr needed

dfuch · 2024-09-03T13:26:05Z

I'll create a CSR once we have converged on the wording.

openjdk · 2024-09-03T13:26:25Z

@dfuch has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@dfuch please create a CSR request for issue JDK-8283779 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

msheppar · 2024-09-03T17:57:20Z

src/java.base/share/classes/java/net/NetworkInterface.java

@@ -97,6 +117,11 @@ public String getName() {
     * {@link NetPermission}("getNetworkInformation") permission, then all
     * InetAddresses are returned.
     *
+     * @implNote
+     * The returned enumeration contains all or a subset of the InetAddresses that were


perhaps additional comma for emphasis
The returned enumeration contains all, or a subset, of the InetAddresses . . .

msheppar · 2024-09-03T17:58:52Z

src/java.base/share/classes/java/net/NetworkInterface.java

@@ -116,6 +141,11 @@ public Enumeration<InetAddress> getInetAddresses() {
     * {@link NetPermission}("getNetworkInformation") permission, then all
     * InetAddresses are returned.
     *
+     * @implNote
+     * The stream contains all or a subset of the InetAddresses that were


perhaps additional comma for emphasis
The stream contains all, or a subset, of the InetAddresses . . .

msheppar

These are good additions and help re-enforce the current apiNote on the equals method and help bring into focus the transient and dynamic nature of NetworkInterface configurations in contemporary network environments.

dfuch · 2024-09-05T18:12:35Z

I have drafted the CSR here: JDK-8339623 - Reviewers are welcome!

src/java.base/share/classes/java/net/NetworkInterface.java

msheppar

FWIW . . . LGTM

dfuch · 2024-09-09T09:58:26Z

I pushed a change that implement feedback received during CSR review. @AlanBateman still good to go?

msheppar · 2024-09-09T10:37:44Z

src/java.base/share/classes/java/net/NetworkInterface.java

+ * looking up a network interface again in order to obtain a new instance.
+ * <p>
+ * Network interface instances are typically used to identify the local
+ * interface on which a multicast group is joined.
 *


Several static methods in this class are factory methods, returning a new instance of a NetworkInterface, reflecting the configuration at the time of instantiation. The network configuration may change at any time, for example DCHP lease renewal , or IPv6 aoutconfig. As such, these methods may need to be invoked again, to obtain a more up-to-date view of the network interfaces.

Factory methods sounds good. Not sure we want to list what could cause the configuration to change: for virtual interfaces created by software components it could be just anything. I'll borrow part of your text and update.

dfuch · 2024-09-10T17:25:57Z

/integrate

openjdk · 2024-09-10T17:27:20Z

Going to push as commit c8e64cb.
Since your change was applied there have been 93 commits pushed to the master branch:

9243104: 8335444: Generalize implementation of AndNode mul_ring
3352522: 8338894: Deprecate jhsdb debugd for removal
be0dca0: 8339698: x86 unused andw/orw/xorw/addw encoding could be removed
64a79d8: 8335625: Update Javadoc for GetCpuLoad
c246ede: 8339799: Reduce work done in j.l.invoke bytecode generators
38441b3: 8339677: [vectorapi] YYYXXXVector::withLaneHelper and laneHelper should use Double::doubleToRawLongBits/Float::floatToRawIntBits
fb51c1e: 8339837: Remove unused BootstrapMethodsInvokers.isLambdaMetafactoryCondyBSM
4d597de: 8338930: StringConcatFactory hardCoded string concatenation strategy
ad10493: 8338526: Don't store abstract and interface Klasses in class metaspace
0d8e52b: 8339800: Prefer invokeBasic in BootstrapMethodInvokers
... and 83 more: https://git.openjdk.org/jdk/compare/ad40a122d632d65052b71125c0dfd58c54e3a521...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-10T17:27:26Z

@dfuch Pushed as commit c8e64cb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8283779

Verified

This commit was signed with the committer’s verified signature.

sergeyklay Serghei Iakovlev

GPG key ID: C6AF1016BBDEA800

Verified
Learn about vigilant mode

7ae62f7

openjdk bot changed the title ~~8283779~~ 8283779: Clarify API documentation of NetworkInterface with respect to configuration changes Sep 2, 2024

openjdk bot added the rfr label Sep 2, 2024

openjdk bot added the net label Sep 2, 2024

dfuch added 2 commits September 2, 2024 16:56

Improve class-level API documentation

5ce21bc

Improve class level description again

74e32e0

AlanBateman reviewed Sep 2, 2024

View reviewed changes

djelinski reviewed Sep 3, 2024

View reviewed changes

src/java.base/share/classes/java/net/NetworkInterface.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/net/NetworkInterface.java Outdated Show resolved Hide resolved

dfuch and others added 2 commits September 3, 2024 12:12

Update src/java.base/share/classes/java/net/NetworkInterface.java

7b647b9

Co-authored-by: Daniel Jelinski <djelinski1@gmail.com>

Update src/java.base/share/classes/java/net/NetworkInterface.java

23bbbea

Co-authored-by: Daniel Jelinski <djelinski1@gmail.com>

dfuch commented Sep 3, 2024

View reviewed changes

dfuch and others added 8 commits September 3, 2024 12:19

Update src/java.base/share/classes/java/net/NetworkInterface.java

6e7f9a8

Update src/java.base/share/classes/java/net/NetworkInterface.java

630496a

Update src/java.base/share/classes/java/net/NetworkInterface.java

549c9b3

Update src/java.base/share/classes/java/net/NetworkInterface.java

b1ca65d

Update src/java.base/share/classes/java/net/NetworkInterface.java

38ccc9d

Update src/java.base/share/classes/java/net/NetworkInterface.java

faf3730

Fix typo

d5c7710

Integrated more feedback

a81ccdc

openjdk bot added the csr label Sep 3, 2024

Merge branch 'master' into NetworkInterface-8283779

7e335f8

msheppar reviewed Sep 3, 2024

View reviewed changes

Review feedback

51d61a7

AlanBateman reviewed Sep 5, 2024

View reviewed changes

src/java.base/share/classes/java/net/NetworkInterface.java Show resolved Hide resolved

dfuch commented Sep 6, 2024

View reviewed changes

src/java.base/share/classes/java/net/NetworkInterface.java Outdated Show resolved Hide resolved

Update src/java.base/share/classes/java/net/NetworkInterface.java

fe9c3a8

AlanBateman approved these changes Sep 6, 2024

View reviewed changes

msheppar approved these changes Sep 8, 2024

View reviewed changes

Feedback from CSR review

cb7c1d5

msheppar reviewed Sep 9, 2024

View reviewed changes

More review feedback

8b9e362

AlanBateman approved these changes Sep 9, 2024

View reviewed changes

openjdk bot added ready and removed csr labels Sep 9, 2024

openjdk bot added the integrated label Sep 10, 2024

openjdk bot closed this Sep 10, 2024

openjdk bot removed ready rfr labels Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8283779: Clarify API documentation of NetworkInterface with respect to configuration changes #20822

8283779: Clarify API documentation of NetworkInterface with respect to configuration changes #20822

dfuch commented Sep 2, 2024 •

edited by openjdk bot

Loading

dfuch commented Sep 2, 2024

bridgekeeper bot commented Sep 2, 2024

openjdk bot commented Sep 2, 2024 •

edited

Loading

openjdk bot commented Sep 2, 2024

openjdk bot commented Sep 2, 2024

mlbridge bot commented Sep 2, 2024 •

edited

Loading

msheppar commented Sep 2, 2024

AlanBateman Sep 2, 2024

AlanBateman Sep 2, 2024

dfuch commented Sep 3, 2024

dfuch commented Sep 3, 2024

openjdk bot commented Sep 3, 2024

msheppar Sep 3, 2024

msheppar Sep 3, 2024

msheppar left a comment •

edited

Loading

dfuch commented Sep 5, 2024 •

edited

Loading

msheppar left a comment

dfuch commented Sep 9, 2024

msheppar Sep 9, 2024 •

edited

Loading

dfuch Sep 9, 2024

dfuch commented Sep 10, 2024

openjdk bot commented Sep 10, 2024

openjdk bot commented Sep 10, 2024

8283779: Clarify API documentation of NetworkInterface with respect to configuration changes #20822

8283779: Clarify API documentation of NetworkInterface with respect to configuration changes #20822

Conversation

dfuch commented Sep 2, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

dfuch commented Sep 2, 2024

bridgekeeper bot commented Sep 2, 2024

openjdk bot commented Sep 2, 2024 • edited Loading

openjdk bot commented Sep 2, 2024

openjdk bot commented Sep 2, 2024

mlbridge bot commented Sep 2, 2024 • edited Loading

Webrevs

msheppar commented Sep 2, 2024

AlanBateman Sep 2, 2024

Choose a reason for hiding this comment

AlanBateman Sep 2, 2024

Choose a reason for hiding this comment

dfuch commented Sep 3, 2024

dfuch commented Sep 3, 2024

openjdk bot commented Sep 3, 2024

msheppar Sep 3, 2024

Choose a reason for hiding this comment

msheppar Sep 3, 2024

Choose a reason for hiding this comment

msheppar left a comment • edited Loading

Choose a reason for hiding this comment

dfuch commented Sep 5, 2024 • edited Loading

msheppar left a comment

Choose a reason for hiding this comment

dfuch commented Sep 9, 2024

msheppar Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

dfuch Sep 9, 2024

Choose a reason for hiding this comment

dfuch commented Sep 10, 2024

openjdk bot commented Sep 10, 2024

openjdk bot commented Sep 10, 2024

dfuch commented Sep 2, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 2, 2024 •

edited

Loading

mlbridge bot commented Sep 2, 2024 •

edited

Loading

msheppar left a comment •

edited

Loading

dfuch commented Sep 5, 2024 •

edited

Loading

msheppar Sep 9, 2024 •

edited

Loading