Opened 3 months ago

Closed 3 months ago

Last modified 7 weeks ago

#22681 closed enhancement (implemented)

adapt onionoo to use metrics-lib 1.9.0

Reported by: iwakeh Owned by: metrics-team
Priority: Medium Milestone: Onionoo-1.3.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Onionoo's tests won't compile w/metrics-lib 1.9.0.

Child Tickets

Change History (9)

comment:1 Changed 3 months ago by iwakeh

Status: newneeds_review

Please review this branch.

comment:2 Changed 3 months ago by karsten

Ah, hang on, I already have temp commits for using metrics-lib 1.9.0 in several applications here. I didn't know you were working on this. Let me push and share my temp branches here in a minute!

comment:3 Changed 3 months ago by iwakeh

Well, that was necessary for pre-release testing.

comment:4 in reply to:  2 Changed 3 months ago by iwakeh

Replying to karsten:

Ah, hang on, I already have temp commits for using metrics-lib 1.9.0 in several applications here. I didn't know you were working on this. Let me push and share my temp branches here in a minute!

The above branch is simply making the tests compile, not using any new ml-190 features.

comment:5 Changed 3 months ago by karsten

Ah, okay!

There, I just pushed task-22141 branches to my Onionoo, metrics-web, and ExoneraTor repositories. Feel free to ignore them for the moment. I'm just trying to avoid duplicate work here.

comment:6 Changed 3 months ago by karsten

Please review my branch task-22681 which is an improved version of my branch above (and which also contains a variant of your patch to make tests compile again).

comment:7 Changed 3 months ago by iwakeh

Tests and checks pass with 1.9.0 and 2.0.0.

I'd like to have more explicit code here, where a null value signals the end of the loop sort of indirectly. Knowing that this also is a matter of taste a first suggestion for more obvious code:

diff --git a/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java b/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java
index ae68b41..9e49840 100644
--- a/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java
+++ b/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java
@@ -129,6 +129,7 @@ class DescriptorQueue {
     }
   }
 
+  /** Returns the next parseable Descriptor. */
   public Descriptor nextDescriptor() {
     Descriptor nextDescriptor = null;
     if (null == this.descriptors) {
@@ -143,14 +144,13 @@ class DescriptorQueue {
         return null;
       }
     }
-    while (null == nextDescriptor && this.descriptors.hasNext()) {
+    while (this.descriptors.hasNext()) {
       nextDescriptor = this.descriptors.next();
-      if (nextDescriptor instanceof UnparseableDescriptor) {
-        nextDescriptor = null;
-        continue;
+      if (!(nextDescriptor instanceof UnparseableDescriptor)) {
+        this.returnedDescriptors++;
+        this.returnedBytes += nextDescriptor.getRawDescriptorLength();
+        break;
       }
-      this.returnedDescriptors++;
-      this.returnedBytes += nextDescriptor.getRawDescriptorLength();
     }
     return nextDescriptor;
   }

All in all merge ready after a change regarding the above topic.

Again, the changes are not at all covered by test, i.e., more intensive pre-deployment-testing is necessary (new ticket).

comment:8 Changed 3 months ago by karsten

Resolution: implemented
Status: needs_reviewclosed

Looks good! Merged to master, together with the update to metrics-lib 2.0.0. Closing. Thanks!

comment:9 Changed 7 weeks ago by iwakeh

Milestone: Onionoo-1.3.0
Note: See TracTickets for help on using tickets.