Opened 2 years ago

Closed 2 years ago

#22652 closed enhancement (implemented)

Adapt CollecTor to metrics-lib 1.9.0

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

Description

This focuses on the changes made in #22141.

Child Tickets

Change History (19)

comment:1 Changed 2 years ago by iwakeh

Status: newneeds_review

Please review task-22652 branch.

comment:2 Changed 2 years ago by karsten

Thanks for that branch! A quick review looks okay, but I'm running into the following exception (with some additional logging output) when syncing bridge descriptors:

2017-06-19 13:27:44,995 ERROR o.t.c.s.SyncPersistence:114 desc.class=BridgeNetworkStatusImpl, filename=statuses
java.lang.ArrayIndexOutOfBoundsException: 2
	at org.torproject.collector.sync.SyncPersistence.storeDesc(SyncPersistence.java:112) ~[collector-1.1.2-dev.jar:1.1.2-dev-15cf44d]
	at org.torproject.collector.sync.SyncManager.mergeWithLocalStorage(SyncManager.java:110) [collector-1.1.2-dev.jar:1.1.2-dev-15cf44d]
	at org.torproject.collector.sync.SyncManager.merge(SyncManager.java:44) [collector-1.1.2-dev.jar:1.1.2-dev-15cf44d]
	at org.torproject.collector.cron.CollecTorMain.run(CollecTorMain.java:76) [collector-1.1.2-dev.jar:1.1.2-dev-15cf44d]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [na:1.8.0_121]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) [na:1.8.0_121]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [na:1.8.0_121]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) [na:1.8.0_121]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:180) [na:1.8.0_121]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293) [na:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [na:1.8.0_121]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [na:1.8.0_121]
	at java.lang.Thread.run(Thread.java:745) [na:1.8.0_121]

It's quite possible that this issue existed before the current change. I didn't spot the bug, but maybe it's easy for you to look? If not, I'll investigate in more detail.

comment:3 Changed 2 years ago by iwakeh

Thanks for checking!
I'll look into this.

comment:4 Changed 2 years ago by iwakeh

Please find the fixup-commit here.

I also added a check for the bridgestatus filename, b/c this could be in the wrong format for other reasons and an error log will be helpful if such a case occurs.

Last edited 2 years ago by iwakeh (previous) (diff)

comment:5 Changed 2 years ago by karsten

Component: Metrics/metrics-libMetrics/CollecTor

Looks pretty good. Please review this additional commit though. Let's revisit once metrics-lib 1.9.0 is out and we're confident that we'll get 2.0.0 out by end of the month. Moving to the CollecTor component.

comment:6 Changed 2 years ago by iwakeh

Status: needs_reviewmerge_ready

This seems fine.
If additional test runs (system tests) don't reveal anything problematic. Another CollecTor release could simply contain the switch to 1.9.0 or/and 2.0.0

comment:7 Changed 2 years ago by karsten

Status: merge_readyneeds_revision

Hmm, I was just about to push to master, but it looks my last patch fixed a regression but at the same time broke the unit tests. I'm not entirely sure what the best fix is there. Would you want to take a look?

comment:8 in reply to:  7 Changed 2 years ago by iwakeh

Replying to karsten:

Hmm, I was just about to push to master, but it looks my last patch fixed a regression but at the same time broke the unit tests. I'm not entirely sure what the best fix is there. Would you want to take a look?

Sure.

comment:9 Changed 2 years ago by iwakeh

Status: needs_revisionneeds_review

Please review this commit. The final fix depends on #22695.

comment:10 Changed 2 years ago by karsten

Please find my task-22652 with a possible fix for the unit tests.

comment:11 Changed 2 years ago by iwakeh

Milestone: CollecTor 1.2.0

comment:12 Changed 2 years ago by karsten

Please find another commit in the same branch with more fixes. Review with care! :)

comment:13 Changed 2 years ago by iwakeh

It's not just unit test; using descriptor-1.9.0 works (please find tiny tweaks for checkstyle and have only one place in the test for tweaking test data).

But, metrics-lib-2.0.0 needs more changes, because the (not previously deprecated) method List<Descriptor> parseDescriptors is gone now.
How should the following problems be adapted?

comment:14 Changed 2 years ago by iwakeh

Status: needs_reviewneeds_revision

Ooops, trac concurrency hickup; let's see, if your second commit fixes what I was complaining about :-)

comment:15 Changed 2 years ago by iwakeh

Status: needs_revisionneeds_review

Yep, I'll review further, but the test tweak from my branch above I'd like to have in.

comment:16 Changed 2 years ago by iwakeh

Status: needs_reviewmerge_ready

Now, things work with both 1.9.0 and more importantly 2.0.0. Checks and tests pass.

Reminder: Deploying this tickets changes needs extensive testing as the bulk of changes is not (yet) covered by tests. (Maybe add a new pre-deploy-test ticket.)

Merge ready.

PS: Would be fine to set the ticket to needs-revision when you start working on it again in order to avoid possible duplicated work (but not too much was duplicated this time).

Last edited 2 years ago by iwakeh (previous) (diff)

comment:17 Changed 2 years ago by karsten

Status: merge_readyneeds_review

Please review my task-22652-3 branch which contains squashed commits from my task-22652 branch plus updates to metrics-lib 2.0.0 and to libraries in Debian stretch.

comment:18 Changed 2 years ago by iwakeh

Status: needs_reviewmerge_ready

Looks fine, all checks and tests pass.

(happy to rebase #22428 on the new master)

comment:19 Changed 2 years ago by karsten

Resolution: implemented
Status: merge_readyclosed

Great, thanks for checking! Pushed to master. Closing.

Note: See TracTickets for help on using tickets.