Opened 3 years ago

Closed 2 years ago

#19607 closed enhancement (implemented)

avoid repeated keyword strings

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

Description

As seen in ServerDescriptorImpl.java

These could be turned into enums.

The issue applies to more than the named class.

Child Tickets

Change History (21)

comment:1 Changed 3 years ago by karsten

Sounds good to me. Though let's keep this out of 1.3.0 and put it into 1.4.0 or whatever the next release will be.

comment:2 Changed 3 years ago by iwakeh

Milestone: metrics-lib 1.4.0

comment:3 Changed 3 years ago by karsten

Milestone: metrics-lib 1.4.0metrics-lib 1.5.0

Move to 1.5.0 in order to get 1.4.0 out sooner.

comment:4 Changed 3 years ago by iwakeh

Milestone: metrics-lib 1.5.0metrics-lib 2.0.0

comment:5 Changed 2 years ago by karsten

Status: newneeds_review

There, I started replacing keywords in server descriptors in my task-19607 branch. But I had trouble with enums and decided to switch over to constants. If you know how to use enums here without bloating the code, please go for it! As soon as we have a solution we like, I'm happy to do the remaining replacements.

comment:6 Changed 2 years ago by karsten

Milestone: metrics-lib 2.0.0metrics-lib 1.7.0

This doesn't seem to be too difficult for the 1.7.0 release. Let's try!

comment:7 Changed 2 years ago by iwakeh

Please see this example-branch using enums. The additional lines will go away when the currently deprecated methods in DescriptorImpl are removed. (I wanted everything to pass tests and checks in this example.)

comment:8 Changed 2 years ago by karsten

Looks good!

Just one question/suggestion: what do you think about moving serverAtMostOnce and other parts that are only relevant for parsing server descriptors to ServerDescriptorImpl?

Other than that, I'm not entirely sure what you mean by "additional lines will go away" in your comment above. Which lines do you mean?

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

Replying to karsten:

Looks good!

Just one question/suggestion: what do you think about moving serverAtMostOnce and other parts that are only relevant for parsing server descriptors to ServerDescriptorImpl?

My reasoning was: if a new field is added in Key the sets for at-most, exactly, etc. are immediately in sight and the new key could be added to the appropriate set(s). But, I don't feel strongly about it; just had to make a choice while coding.

Other than that, I'm not entirely sure what you mean by "additional lines will go away" in your comment above. Which lines do you mean?

Sounds a little cryptic :-) The lines listed as new in the git stats. I'm referring to protected void check*Keywords(Set<String> keywords) methods, which could be replaced by protected void check*Keys(Set<Key> keys) eventually.

comment:10 in reply to:  9 Changed 2 years ago by karsten

Status: needs_reviewneeds_revision

Replying to iwakeh:

Replying to karsten:

Looks good!

Just one question/suggestion: what do you think about moving serverAtMostOnce and other parts that are only relevant for parsing server descriptors to ServerDescriptorImpl?

My reasoning was: if a new field is added in Key the sets for at-most, exactly, etc. are immediately in sight and the new key could be added to the appropriate set(s). But, I don't feel strongly about it; just had to make a choice while coding.

Okay, in that case I'd prefer if we keep descriptor-related things in the respective descriptor classes. Both would work, but that seems potentially less confusing in the long run.

Other than that, I'm not entirely sure what you mean by "additional lines will go away" in your comment above. Which lines do you mean?

Sounds a little cryptic :-) The lines listed as new in the git stats. I'm referring to protected void check*Keywords(Set<String> keywords) methods, which could be replaced by protected void check*Keys(Set<Key> keys) eventually.

Makes more sense!

How do we proceed? Do you want to apply these changes to all descriptors, and I review those changes, or the other way around?

comment:11 Changed 2 years ago by iwakeh

Owner: changed from karsten to iwakeh
Status: needs_revisionaccepted

I can continue here.

comment:12 Changed 2 years ago by iwakeh

Milestone: metrics-lib 1.7.0metrics-lib 1.8.0

comment:13 Changed 2 years ago by iwakeh

FYI:
key geoip6-bd-digest is missing from at-most-once-keys (spec); unless there is a veto I'll add it in this ticket w/o further notice.

comment:14 Changed 2 years ago by karsten

Sure, please do.

comment:15 Changed 2 years ago by iwakeh

Pheeew, that took longer than anticipated. Please review my branch; checks and tests pass.

(I noticed many places where the interface clean-up will be helpful, but we have a different ticket for that.)

comment:16 Changed 2 years ago by iwakeh

Status: acceptedneeds_review

comment:17 Changed 2 years ago by karsten

Looks good!

I fixed a tiny bug where a space character got lost:

@@ -66,8 +66,8 @@ public class RelayNetworkStatusConsensusImpl extends NetworkStatusImpl
   private void calculateDigest() throws DescriptorParseException {
     try {
       String ascii = new String(this.getRawDescriptorBytes(), "US-ASCII");
-      String startToken = "network-status-version ";
-      String sigToken = "\ndirectory-signature ";
+      String startToken = Key.NETWORK_STATUS_VERSION.keyword;
+      String sigToken = "\ndirectory_signature ";
       if (!ascii.contains(sigToken)) {
         return;
       }

And I added two more commits in an attempt to make it look even better and possibly even save memory by storing everything in EnumSet<Key> rather than Set<String>. Please review my branch task-19607-3 carefully!

comment:18 in reply to:  17 Changed 2 years ago by iwakeh

Replying to karsten:

Looks good!

I fixed a tiny bug where a space character got lost:

...

And I added two more commits in an attempt to make it look even better and possibly even save memory by storing everything in EnumSet<Key> rather than Set<String>. Please review my branch task-19607-3 carefully!

Thanks for fixing the space and adding the missed constants/enums!

I found another place for use of constants. In declarations we should use interfaces if possible.

Please find these changes for review here (two more commits).

comment:19 Changed 2 years ago by karsten

Status: needs_reviewmerge_ready

Both commits look good to me! Would you want to squash all commits starting with d96133e into one? Feel free to --reset-author, as the bulk of the work was done by you. Thanks!

comment:20 Changed 2 years ago by iwakeh

Please check the one commit including all. I also adapted the changelog a bit.

comment:21 Changed 2 years ago by karsten

Resolution: implemented
Status: merge_readyclosed

Cherry-picked, rebased to master, and pushed. Please re-open if I screwed up somewhere in that process. Closing. Thanks!

Note: See TracTickets for help on using tickets.