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

Gh 2552 remove deprecated #2564

Merged
merged 10 commits into from
Feb 23, 2022
Merged

Gh 2552 remove deprecated #2564

merged 10 commits into from
Feb 23, 2022

Conversation

t92549
Copy link
Contributor

@t92549 t92549 commented Jan 14, 2022

As of submitting, this branch is not yet complete, still todo:

Some deprecated code has not been removed in this PR but will be removed under a different PR due to its dependency on the future FederatedStore changes. This is to do with replacing usages of the getTraits function:

Related Issue

* It has been marked deprecated but will not be removed as it is needed
* in the Serialiser interface.
* @see #deserialise(byte[], int, int)
*/
@Deprecated
@Override
public final T deserialise(final byte[] bytes) throws SerialisationException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function, as well as ToBytesSerialiser.deserialise(byte[]), have been left in despite being deprecated. This is because they cannot be removed as they need to have implementations for the Serialiser interface. However, the deprecated tag has been left in to warn people to instead use the more efficient deserialise(byte[], int, int) method instead.

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2022

Codecov Report

❗ No coverage uploaded for pull request base (v2-alpha@6c0be90). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             v2-alpha    #2564   +/-   ##
===========================================
  Coverage            ?   65.28%           
  Complexity          ?     2427           
===========================================
  Files               ?      872           
  Lines               ?    27209           
  Branches            ?     3165           
===========================================
  Hits                ?    17764           
  Misses              ?     8046           
  Partials            ?     1399           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c0be90...553412c. Read the comment docs.

@t92549
Copy link
Contributor Author

t92549 commented Jan 14, 2022

The Parquet and HBase store have been removed in this PR as we plan to first release 2.0 without them and add them back in later if there is user demand

@t92549 t92549 linked an issue Jan 14, 2022 that may be closed by this pull request
Copy link
Member

@GCHQDeveloper314 GCHQDeveloper314 left a comment

Choose a reason for hiding this comment

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

It looks like these files have been left over from the parquet removal:

/example/road-traffic/road-traffic-demo/src/main/resources/parquet/store.properties
/example/road-traffic/road-traffic-rest/src/test/resources/parquet/store.properties
/example/basic/basic-rest/src/main/resources/parquet/store.properties

I went through most of deprecation changes whilst documenting them and it all looked fine to me.

* gh-2573: Replace hasTrait usages with Operation

* gh-2573: Fix tests by making exception more explicit

* gh-2573: Fixed exception issues

* gh-2573: Simplified but still failing test

* gh-2573: Changed to user

* gh-2573: Changed FederatedGetTraitsHandler to return DYNAMIC_SCHEMA

* gh-2573: Use old function for FederatedOperationChainValidator

* gh-2573: Fixed FederatedStoreUtil

* gh-2573: Added Deprecation notice

* gh-2573: Cherry pick federated store tests fix from 2553

* Revert "gh-2573: Cherry pick federated store tests fix from 2553"

This reverts commit c367555.

* gh-2573: Manual fix of FederatedStoreGetTraitsTest cache

* gh-2573: Changed HasTraitHandler to not send Federated loop option
@t11947 t11947 self-requested a review January 31, 2022 10:01
Copy link
Contributor

@t11947 t11947 left a comment

Choose a reason for hiding this comment

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

Just a single comment regarding reference to a specific store in general store code, all good apart from that.

@t11947 t11947 self-requested a review February 2, 2022 16:30
@t11947 t11947 marked this pull request as ready for review February 2, 2022 16:30
@t92549
Copy link
Contributor Author

t92549 commented Feb 3, 2022

Not to be merged until #2579 is merged into this one

p3430233 and others added 2 commits February 4, 2022 11:46
* Update docs links in README

* Revert "Update docs links in README"

This reverts commit 2d87bf3.

* gh-2559 added hasTrait, its handler and corresponding tests (#2561)

* gh-2559 added hasTrait, its handler and corresponding tests

* gh-2559 amended comment in HasTrait, added additional tests for the handler

* gh-2559 null check & tidy test

Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>

* Update links to Gaffer docs in all files (#2566)

Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>

* gh-2560 GetTraitsHandler is initalised with traits dependednt on store type

* gh-2560 reviewed comments

* gh-2560 unused imports

* gh-2560 review comments

* checkstyle

* gh-2560 fixed federated test

* gh-2560 fixed GraphTest

* gh-2560 fixed TestStore

* gh-2560-getTraitsOperation (#2582)

Removing redundant adding of GetTraitHandler from addAdditionalOperationHandlers() because it has its own method getGetTraitsHandler().

* gh-2560: Added fix to SingleUseMapStoreWithoutVisibilitySupport

* gh-2560: Fixed Set copy issue in GetTraitsHandler constructor

* gh-2560: Added extra protections to GetTraitsHandler

Co-authored-by: GCHQDeveloper314 <94527357+GCHQDeveloper314@users.noreply.github.com>
Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>
Co-authored-by: GCHQDev404 <45399082+GCHQDev404@users.noreply.github.com>
Copy link
Contributor

@GCHQDev404 GCHQDev404 left a comment

Choose a reason for hiding this comment

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

Awaiting last PR "multi serialiser comments"

.view(viewBuilder.build())
.seedMatching(seedMatching)
.build();
if (createChain && includeEntities && includeEdges) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Notes need to be added to Deprecated documents.

public class AccumuloElementsRetriever extends AccumuloSingleIDRetriever<GetElements> {
public AccumuloElementsRetriever(final AccumuloStore store,
final GetElements operation,
final User user)
throws IteratorSettingException, StoreException {
super(store, operation, user,
SeedMatching.SeedMatchingType.EQUAL != operation.getSeedMatching(),
// includeMatchedVertex if input only contains EntityIds
StreamSupport.stream(operation.getInput().spliterator(), false).noneMatch(input -> EdgeId.class.isInstance(input)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially input could be large, but its a simple stream with an end-quick if any is matched.
IF your seed input is massive, you likely to have issues else where in gaffer.

assertThat(edgeResults).hasSize(1);
assertThat(edgeResults).doNotHave(matchedVertex);
} else if (edge == getEdgeWithDestinationMatch()) {
// When the Edge input matches on the dest and the Entity input matches on
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add to documents?

* gh-2552-remove-deprecated MultiSerialiser

update the MultiSerialiser wanting to emphasise the effectiveness of it during the deprecation of serialisers.
8 is the last and best.
24 is not latest regardless of natural-order.
7 is dead.

* gh-2552: MultiSerialiser spelling corrections

* Update core/serialisation/src/main/java/uk/gov/gchq/gaffer/serialisation/implementation/MultiSerialiser.java

Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>
Copy link
Member

@GCHQDeveloper314 GCHQDeveloper314 left a comment

Choose a reason for hiding this comment

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

All looks good to me now. I've also somewhat tested this branch by using it in my own branch.

@t92549 t92549 merged commit dd18afb into v2-alpha Feb 23, 2022
@t92549 t92549 linked an issue Feb 23, 2022 that may be closed by this pull request
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.

Remove Parquet store. Remove references to Parquet from examples/demos Remove Deprecated code
7 participants