Skip to content

GEOWAVE-306#307

Merged
chrisbennight merged 1 commit intomasterfrom
GEOWAVE-306
Apr 4, 2015
Merged

GEOWAVE-306#307
chrisbennight merged 1 commit intomasterfrom
GEOWAVE-306

Conversation

@rwgdrummer
Copy link
Copy Markdown
Contributor

@rwgdrummer rwgdrummer commented Apr 2, 2015

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 2, 2015

Coverage Status

Coverage increased (+0.07%) to 31.82% when pulling 573e3f6 on GEOWAVE-306 into f481485 on master.

chrisbennight
chrisbennight reviewed Apr 3, 2015
View reviewed changes
Comment thread ...nalytics/src/main/java/mil/nga/giat/geowave/analytics/clustering/CentroidManagerGeoWave.java Outdated
Copy link
Copy Markdown
Contributor

@chrisbennight chrisbennight Apr 3, 2015

Choose a reason for hiding this comment

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

What's the intent behind converting these exceptions all to IOException?

Copy link
Copy Markdown
Contributor Author

@rwgdrummer rwgdrummer Apr 3, 2015

Choose a reason for hiding this comment

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

Used in situations such as setup in Mapper/Reducers that only handles IOException. Rather littering the method signature and pushing the redundant handling else where, I do it here.

Copy link
Copy Markdown
Contributor

@chrisbennight chrisbennight Apr 3, 2015

Choose a reason for hiding this comment

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

Gotcha - I understand it in the case of a constrained method that needs to be implemented/overrided - just didn't see that here. What does e.getMessage() do in this case? Is there any value in something like

throw new IOException(e, e.getMessage())

(might be pointless - intent here is just to make sure we aren't slightly obfuscating the InstatiantionException of IllegalAcessException, since those are more useful than IOException)

Copy link
Copy Markdown
Contributor Author

@rwgdrummer rwgdrummer Apr 3, 2015

Choose a reason for hiding this comment

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

yep.

@rwgdrummer
Copy link
Copy Markdown
Contributor Author

rwgdrummer commented Apr 3, 2015

I started looking at this again more closely.
PropertyManagement changed a bit for me as I started seeing its potential as a uniform way to handle parameters in the analytics. Looking at it some more tonight, I think there is some thought that I left unfinished. I am going to make some additional adjustments tomorrow.

@rwgdrummer
Copy link
Copy Markdown
Contributor Author

rwgdrummer commented Apr 3, 2015

Addressed the concerns of the last push.
(1) Added localized message to IOException
(2) Consistent handling of serializable objects in the parameter management.
(3) formatting efficiencies.
So, ready for another review.
Thanks

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 4, 2015

Coverage Status

Coverage increased (+0.37%) to 32.12% when pulling 905903e on GEOWAVE-306 into f481485 on master.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 4, 2015

Coverage Status

Coverage increased (+0.49%) to 32.24% when pulling 905903e on GEOWAVE-306 into f481485 on master.

chrisbennight added a commit that referenced this pull request Apr 4, 2015
@chrisbennight
Merge pull request #307 from ngageoint/GEOWAVE-306
257845b 
Fix issue where WFS-T. BBOX parameter fails in tablet with GeoServer 2.6.x #306
@chrisbennight chrisbennight merged commit 257845b into master Apr 4, 2015
@rfecher rfecher deleted the GEOWAVE-306 branch May 5, 2015 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rwgdrummer @coveralls @chrisbennight @rfecher @jwomeara