Skip to content

JAVA-444: Add Java process information to UUIDs.makeNode() hash.#593

Merged
olim7t merged 2 commits into2.1from
java444
Mar 9, 2016
Merged

JAVA-444: Add Java process information to UUIDs.makeNode() hash.#593
olim7t merged 2 commits into2.1from
java444

Conversation

@adutra
Copy link
Copy Markdown
Contributor

@adutra adutra commented Feb 15, 2016

This is based on #591 and supersedes #521.

@adutra adutra added this to the 2.1.10 milestone Feb 15, 2016
@adutra adutra mentioned this pull request Feb 15, 2016
Closed
@adutra
Copy link
Copy Markdown
Contributor Author

adutra commented Feb 15, 2016

Benchmark under Windows getpid() vs JMX:

Benchmark               Mode  Cnt      Score     Error  Units
Benchmark.testJMX  avgt   20  20070.951 ± 406.583  ns/op
Benchmark.testJNR  avgt   20     66.059 ±   1.798  ns/op

@adutra
Copy link
Copy Markdown
Contributor Author

adutra commented Feb 16, 2016

Improved documentation for Native and UUIDs classes. Squashed commits again.

@adutra adutra mentioned this pull request Feb 19, 2016
Merged
olim7t
olim7t reviewed Feb 22, 2016
View reviewed changes
Comment thread driver-core/src/main/java/com/datastax/driver/core/utils/UUIDs.java Outdated
* <ol>
* <li>Since Java does not provide direct access to the host's MAC address, that information
* is replaced with a digest of all IP addresses available on the host;</li>
* <li>The process ID (PID) isn't easily available to Java neither, so it is determined by one of the
Copy link
Copy Markdown
Contributor

@olim7t olim7t Feb 22, 2016

Choose a reason for hiding this comment

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

isn't available ... either

@olim7t
Copy link
Copy Markdown
Contributor

olim7t commented Feb 22, 2016

Looks good to me, apart from 2 minor remarks and the "optional" discussion.

@olim7t
Copy link
Copy Markdown
Contributor

olim7t commented Feb 23, 2016

Rebased on current 2.1 (2708bd7)

@adutra adutra force-pushed the java444 branch 2 times, most recently from f3f7d1e to 1c22a13 Compare March 1, 2016 15:22
@adutra
Copy link
Copy Markdown
Contributor Author

adutra commented Mar 1, 2016

Rebased again on top of current #591.

tolbertam
tolbertam reviewed Mar 2, 2016
View reviewed changes
Comment thread driver-core/src/main/java/com/datastax/driver/core/utils/UUIDs.java
}
if (pid == null) {
try {
String pidJmx = ManagementFactory.getRuntimeMXBean().getName().split("@")[0];
Copy link
Copy Markdown
Contributor

@tolbertam tolbertam Mar 2, 2016

Choose a reason for hiding this comment

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

Really hard to test this as is as I need a non-POSIX system :D, I commented out the code above and it seems to work 👍

@tolbertam tolbertam mentioned this pull request Mar 2, 2016
Merged
tolbertam
tolbertam reviewed Mar 2, 2016
View reviewed changes
Comment thread driver-core/src/test/java/com/datastax/driver/core/MockClocks.java

@Override
public long currentTime() {
public long currentTimeMicros() {
Copy link
Copy Markdown
Contributor

@tolbertam tolbertam Mar 2, 2016

Choose a reason for hiding this comment

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

We could probably remove the 'public' modifiers from these since the parent class is not public right? (we'd probably need to add from clirr exceptions though, doesn't seem important).

Copy link
Copy Markdown
Contributor Author

@adutra adutra Mar 3, 2016

Choose a reason for hiding this comment

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

Unfortunately that would not work here, as this method is inherited from Clock interface, so it needs to be public.

@adutra
Copy link
Copy Markdown
Contributor Author

adutra commented Mar 4, 2016

Rebased on top of current #591 to benefit from OSGi fixes.
Changed the loading process of JNR POSIX library in Native class to use reflection; otherwise it would fail in an unexpected manner when this library is not present in the classpath.

@tolbertam
Copy link
Copy Markdown
Contributor

tolbertam commented Mar 4, 2016

This looks good to me 👍, think it can be safely merged after #591 is settled/merged (or if you just want to merge this containing #591).

@tolbertam
Copy link
Copy Markdown
Contributor

tolbertam commented Mar 9, 2016

👍 on merging after rebased on 2.1 after #591 is merged.

olim7t added a commit that referenced this pull request Mar 9, 2016
@olim7t
Merge pull request #593 from datastax/java444
da7d95b 
JAVA-444: Add Java process information to UUIDs.makeNode() hash.
@olim7t olim7t merged commit da7d95b into 2.1 Mar 9, 2016
@olim7t olim7t deleted the java444 branch March 9, 2016 21:28
Sfurti-yb pushed a commit to yugabyte/cassandra-java-driver that referenced this pull request Dec 8, 2023
Polishing.
f35c579 
Javadoc param description.

[apache#593]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

reviewed_AT

Projects

None yet

Milestone

2.1.10

Development

Successfully merging this pull request may close these issues.

3 participants

@adutra @olim7t @tolbertam