Opened 9 years ago

Closed 9 years ago

#1039 closed enhancement (complete)

Modify resolver messages

Reported by: stephen Owned by: stephen
Priority: low Milestone: Sprint-20110628
Component: logging Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0.5 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by stephen)

Same changes as described in #1034 but for the files in src/bin/resolver.

Subtickets

Change History (7)

comment:1 Changed 9 years ago by stephen

  • Owner set to stephen
  • Status changed from new to assigned

comment:2 Changed 9 years ago by stephen

  • Owner changed from stephen to UnAssigned
  • Status changed from assigned to reviewing

comment:3 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:4 Changed 9 years ago by vorner

  • Owner changed from vorner to stephen

Hello

I noticed some minor things:

  • RESOLVER_PROTOCOL_ERROR is used twice.
  • RESOLVER_RECQ_SETUP is still abbreviated, what does the REC mean?
  • RESOLVER_NOT_ONE_QUEST still abbreviated, quest being a valid word, it looks strange.
  • It took me a while to find out TMO is not a typo of TWO, but TIMEOUT. Maybe use just TIME instead of TMO? It is much more readable (at last for me, as non-native speaker).

Thanks

comment:5 follow-up: Changed 9 years ago by stephen

  • Component changed from Unclassified to logging
  • Estimated Difficulty changed from 0.0 to 0.5
  • Owner changed from stephen to vorner

RESOLVER_PROTOCOL_ERROR is used twice.

Well-spotted. Changed the second occurrence to RESOLVER_MESSAGE_ERROR.

RESOLVER_RECQ_SETUP is still abbreviated, what does the REC mean?

RECQ was an abbreviation for RecursiveQuery: I've changed it to QUERY. (There is a naming issue in the resolver library where there are messages for RecursiveQuery and RunningQuery objects. However, as only one is mentioned here, QUERY is suitably non-ambiguous.)

RESOLVER_NOT_ONE_QUEST still abbreviated, quest being a valid word, it looks strange.

and

It took me a while to find out TMO is not a typo of TWO, but TIMEOUT. Maybe use just TIME instead

All these messages are a trade-off between readability and the length of the identifier: at what point do we decide it is too long? However, as I added the identifiers UNEXPECTED_RESPONSE and UNSUPPORTED_OPCODE, I've used these as a benchmark for maximum length and was able to expand more or less all abbreviations.

One option would be to try to reduce the length of the prefix - would it be worth coming up with a three- or four-letter abbreviation for all of the different modules (e.g. abbreviating RESOLVER_ to RES_)?

comment:6 in reply to: ↑ 5 Changed 9 years ago by vorner

  • Owner changed from vorner to stephen

Hello

Replying to stephen:

RESOLVER_NOT_ONE_QUEST still abbreviated, quest being a valid word, it looks strange.

and

It took me a while to find out TMO is not a typo of TWO, but TIMEOUT. Maybe use just TIME instead

All these messages are a trade-off between readability and the length of the identifier: at what point do we decide it is too long? However, as I added the identifiers UNEXPECTED_RESPONSE and UNSUPPORTED_OPCODE, I've used these as a benchmark for maximum length and was able to expand more or less all abbreviations.

Yes, I know it doesn't look good if they are too long. I'm not against abbreviations in case it is well-known (eg. SRV). And it probably isn't that important, if there's the description besides the identifier, just that when we are at it, I noted the ones which looked unknown to me.

One option would be to try to reduce the length of the prefix - would it be worth coming up with a three- or four-letter abbreviation for all of the different modules (e.g. abbreviating RESOLVER_ to RES_)?

Hmm, yes, that seems reasonable. I'm just not sure if we aren't fine-tuning the outputs too much O:-).

It seems OK to merge.

With regards

comment:7 Changed 9 years ago by stephen

  • Description modified (diff)
  • Resolution set to complete
  • Status changed from reviewing to closed

Merged into master, commit 5355f8f14648fddf13cda7240530e7b4216da671

Note: See TracTickets for help on using tickets.