Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lists for RRset #56

Merged
merged 2 commits into from Jul 14, 2019

Conversation

@ibauersachs
Copy link
Member

commented Jun 3, 2019

Do you agree with the new API on RRset? This is the first real breaking change. I thought about adding an overload or making rrs() generic if the set is expected to contain only one type.

@ibauersachs ibauersachs requested a review from kingle Jun 3, 2019

@bwelling

This comment has been minimized.

Copy link
Collaborator

commented Jun 3, 2019

Is there some reason to return a List instead of an Iterator? That seems like extra copying.

RRSets always contain records of only one type (not including signatures, which are separate).

@@ -150,32 +151,35 @@
list.addAll(rrs.subList(start, total));
}

return list.iterator();
return list;
}

/**
* Returns an Iterator listing all (data) records.

This comment has been minimized.

Copy link
@kingle

kingle Jun 3, 2019

Collaborator

Update javadoc to reflect new change to return type

}

/**
* Returns an Iterator listing all (data) records.
* @param cycle If true, cycle through the records so that each Iterator will
* start with a different record.
*/
public synchronized Iterator<Record>
public synchronized List<Record>
rrs(boolean cycle) {

This comment has been minimized.

Copy link
@kingle

kingle Jun 3, 2019

Collaborator

I'm definitely fine with making this change to use Lists.

I'm not going to stop a breaking change, given this is a major version bump, but another possible way forward without breaking old consumers is to introduce new method(s) with different names, getRRs or getDataRecords (or whatever you want to name them) that uses the new List return type. Then add the @deprecated annotation and javadoc tag to the old Iterator-based rss() methods so consumers know to use the new ones (and switch all internal code to use the new ones). If you go that route, apply the same to sigs -> getSigs() and elsewhere.

Just throwing that out there.

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 4, 2019

Author Member

I thought about keep the existing methods as deprecated, but really liked their short and concise name. While still standard Java, getRrs and getSigs is somewhat cumbersome. Not really an argument, I know.
If consumers don't want to refactor their existing iterator code to for-each's, a simple regex - s/\.rrs\(\)/\.rrs\(\)\.iterator\(\)/ - will do. So there's not much refactoring involved.

Not sure yet if it's possible to make the entire RRset class generic. But the casting that I need to do everywhere in dnssecjava is reason enough to try.

@ibauersachs

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

@bwelling Right, seems it was a little late when I wrote that. The type-bitmap in RRSIGs must have confused me. The lists aren't copied again, that is already being done in the existing private rrs(boolean, boolean) to split data from sigs. There could be two separate internal Lists to make this unnecessary in the non-cycling case.
Do you remember the intention behind the cycling? I understand why that needs to be done in general, but not why this shouldn't be application specific.

@bwelling

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

I'm not sure why the rrs and sigs lists are separate. It's possible that it was a misguided optimization, but it would definitely make sense to split them.

The optional cycling is there because iterating through records in a nondeterministic order is often useful, so it seemed reasonable to make it part of the RRset iteration. If I was designing the API from scratch, I'm not sure if it would still make sense or not (it should be somewhere, but that might njot be the right place), and I definitely wouldn't implement the existing cyclic behavior that requires a write lock to maintain the position counter.

@ibauersachs ibauersachs force-pushed the rrset-list branch 2 times, most recently from 6ef67a3 to 9d43d33 Jun 17, 2019

@ibauersachs ibauersachs force-pushed the rrset-list branch from 9d43d33 to e6e2464 Jun 18, 2019

@@ -364,15 +365,15 @@
getSectionRRsets(int section) {
if (sections[section] == null)
return emptyRRsetArray;
List<RRset> sets = new LinkedList<>();
List<RRset<Record>> sets = new LinkedList<>();

This comment has been minimized.

Copy link
@kingle

kingle Jun 22, 2019

Collaborator

Is there a particular reason to use a LinkedList here instead of ArrayList?

This comment has been minimized.

Copy link
@ibauersachs

ibauersachs Jun 22, 2019

Author Member

I don't think so. It's been like that for 17 years...

@kingle

kingle approved these changes Jun 22, 2019

@ibauersachs ibauersachs merged commit f237c5b into master Jul 14, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.03%) to 46.571%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.