Skip to content

Conversation

@kevinrr888
Copy link
Member

closes #4038

InstanceOperations:
Did not find any changes needed to be made:

  • modifyProperties() returns non-composite (System level only) view as
    stated in the javadocs
  • getSystemConfiguration() returns composite view as stated in the
    javadocs
  • getSiteConfiguration() returns non-composite (Site level only) view as
    stated in the javadocs

NamespaceOperations:

  • modifyProperties() returns non-composite (Namespace level only) view
    as stated in the javadocs (no changes here)
  • getProperties() returns a composite view. Updated javadocs.
  • getConfiguration() returns a composite view. Updated javadocs.
  • getNamespaceProperties() returns a non-composite (Namespace level
    only) view. Updated javadocs.

TableOperations:

  • modifyProperties() returns non-composite (Table level only) view
    as stated in the javadocs (no changes here)
  • getProperties() returns a composite view. Updated javadocs.
  • getConfiguration() returns a composite view. Updated javadocs.
  • getTableProperties() returns a non-composite (Table level
    only) view. Updated javadocs.

No other methods were applicable.

@EdColeman
Copy link
Contributor

Do you have extra files in this commit? It looks like you may have include the changes for ScanConsistencyIT.

@kevinrr888
Copy link
Member Author

I think I must have branched off my other feature branch, not main. Working on fix

Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made one suggestion in one place, but it might apply to other places as well... I just wasn't going to be comment the same thing in multiple places.

It looks like this is targeting the main branch. The PR should be targeting the 2.1 branch instead, as these methods exist there as well.

InstanceOperations:
Did not find any changes needed to be made:
- modifyProperties() returns non-composite (System level only) view as
  stated in the javadocs
- getSystemConfiguration() returns composite view as stated in the
  javadocs
- getSiteConfiguration() returns non-composite (Site level only) view as
  stated in the javadocs
NamespaceOperations:
- modifyProperties() returns non-composite (Namespace level only) view
  as stated in the javadocs (no changes here)
- getProperties() returns a composite view. Updated javadocs.
- getConfiguration() returns a composite view. Updated javadocs.
- getNamespaceProperties() returns a non-composite (Namespace level
  only) view. Updated javadocs.
TableOperations:
- modifyProperties() returns non-composite (Table level only) view
  as stated in the javadocs (no changes here)
- getProperties() returns a composite view. Updated javadocs.
- getConfiguration() returns a composite view. Updated javadocs.
- getTableProperties() returns a non-composite (Table level
  only) view. Updated javadocs.

No other methods were applicable.
* method returns a Map instead of an Iterable.
* Gets a merged view of the properties of a namespace from its parent configuration. These
* properties are inherited by tables in this namespace. Note that recently changed properties may
* not be available immediately. This new method returns a Map instead of an Iterable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel that the This new method... lines should be included. They would only be valid for one version, until the are no longer "new". IDE's will highlight the return type when someone tries to use it. Feel free to ignore if it does seem useful and I'm missing that context.

Copy link
Member Author

@kevinrr888 kevinrr888 Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree. I found it strange that "new" was included. It was already present and I didn't want to make any unwanted changes. I'll remove it

@kevinrr888 kevinrr888 changed the base branch from main to 2.1 December 19, 2023 19:31
@kevinrr888 kevinrr888 changed the base branch from 2.1 to main December 19, 2023 19:32
@kevinrr888 kevinrr888 changed the base branch from main to 2.1 December 19, 2023 19:55
@kevinrr888
Copy link
Member Author

Changed to target 2.1 branch, not main

@ctubbsii
Copy link
Member

It seemed odd that this didn't include changes to InstanceOperations, so I went and looked and it seems we don't have a method to retrieve the non-merged view of the system configuration like we do for namespace and tables. That's a frustrating inconsistency, but it shouldn't be added in a bugfix release. It can be addressed in 3.1, separate from this javadoc update, if we want to add that.

@kevinrr888
Copy link
Member Author

It seemed odd that this didn't include changes to InstanceOperations, so I went and looked and it seems we don't have a method to retrieve the non-merged view of the system configuration like we do for namespace and tables. That's a frustrating inconsistency, but it shouldn't be added in a bugfix release. It can be addressed in 3.1, separate from this javadoc update, if we want to add that.

Yeah, I was also wondering that. Let me know if you think I should open an issue for that

@keith-turner
Copy link
Contributor

@ctubbsii are you ok with these changes now?

Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keith-turner asked:

@ctubbsii are you ok with these changes now?

Yeah, seems fine. My previous rejection was only because of the need to change the base branch to 2.1.

I see my suggested wording about the parent configuration was also added, but after looking at it in context, I'm wondering if it should say "with its parent configuration" rather than "from its parent configuration". Regardless, I'm fine with these changes, with or without additional tweaks to that phrasing.

@kevinrr888
Copy link
Member Author

I also think 'with' is better. Changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update configuration javadoc to indicate if a composite view is being returned or not.

5 participants