Task/refactor proto handler #1830

hlgp · 2023-01-31T02:59:47Z

Some efficiency gains by not performing JexlEvaluation for all edge definitions on an event that doesn't even contain the necessary fields, general cleanup, etc.

…SecurityAgency/datawave into bugfix/edgePrecondition

Conflicts: warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

...t-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

…alSecurityAgency/datawave into task/refactorProtoHandler

* Fix nested field group names, edge efficiencies * More efficient pattern check plus tests * formatter * modifications for trim and extract methods * PR feedback * Address #1905 more surgical approach to group trimming for edge generation * Address #1905 MR comments --------- Co-authored-by: hlgp <hlgp@users.noreply.github.com> Co-authored-by: hgklohr <hgklohr@gmail.com> Conflicts: warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java warehouse/ingest-core/src/test/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDeleteModeTest.java

Conflicts: warehouse/ingest-core/src/main/java/datawave/ingest/data/TypeRegistry.java warehouse/ingest-core/src/main/java/datawave/ingest/data/normalizer/SimpleGroupFieldNameParser.java warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/define/EdgeDataBundle.java warehouse/ingest-core/src/test/java/datawave/ingest/data/normalizer/SimpleGroupFieldNameParserTest.java warehouse/ingest-core/src/test/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDeleteModeTest.java warehouse/ingest-csv/src/main/java/datawave/ingest/csv/config/helper/ExtendedCSVIngestHelper.java warehouse/ingest-json/src/main/java/datawave/ingest/json/config/helper/JsonIngestHelper.java

Conflicts: warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

Conflicts: warehouse/ingest-configuration/src/main/resources/config/edge-definitions.xml warehouse/ingest-configuration/src/main/resources/config/edge-ingest-config.xml warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java warehouse/ingest-core/src/test/java/datawave/ingest/mapreduce/handler/edge/EdgeHandlerTestUtil.java warehouse/ingest-core/src/test/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgePreconditionTest.java warehouse/ingest-core/src/test/resources/config/edge-ingest-config.xml warehouse/ingest-csv/src/test/resources/config/ingest/edge-ingest-config.xml warehouse/ingest-json/src/test/resources/config/ingest/edge-ingest-config.xml

matthpeterson · 2024-02-07T14:44:25Z

...se/ingest-core/src/test/java/datawave/ingest/mapreduce/handler/edge/EdgeHandlerTestUtil.java

@@ -1,7 +1,10 @@
 package datawave.ingest.mapreduce.handler.edge;

 import java.io.IOException;
+import java.util.ArrayList;


These new imports appear to be unused.

I put this and a few other suggested removals in https://github.com/NationalSecurityAgency/datawave/pull/2269/files

matthpeterson · 2024-02-07T14:49:24Z

.../ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/define/EdgeDataBundle.java

@@ -97,6 +99,17 @@ public EdgeDataBundle(EdgeDefinition edgeDef, NormalizedContentInterface ifaceSo
        this.initMarkings(getSource().getMarkings(), getSink().getMarkings());


The constructor public EdgeDataBundle(EdgeDefinition edgeDef... can be deleted now that it is unused. Its code now seems to exist in the data type handler's createEdge.

I put this and a few other suggested removals in https://github.com/NationalSecurityAgency/datawave/pull/2269/files

matthpeterson · 2024-02-07T14:51:39Z

...t-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

@@ -983,28 +623,6 @@ private NormalizedContentInterface getNullKeyedNCI(String fieldValue, Multimap<S
        return null;


getNullKeyedNCI in PEDTHandler.java is now unused and can be deleted.

I put this and a few other suggested removals in https://github.com/NationalSecurityAgency/datawave/pull/2269/files

matthpeterson · 2024-02-07T15:33:33Z

.../ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/define/EdgeDataBundle.java

+    public void setEdgeDefinition(EdgeDefinition edgeDef) {
+        this.edgeDefinition = edgeDef;
+        this.edgeDirection = edgeDef.getDirection();
+        this.edgeType = edgeDef.getEdgeType().toString();


Suggested change

this.edgeType = edgeDef.getEdgeType().toString();

this.edgeType = edgeDef.getEdgeType();

matthpeterson · 2024-02-07T15:41:23Z

...st-core/src/test/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDeleteModeTest.java

@@ -243,141 +230,12 @@ public void setup(TaskAttemptContext context) {
        public long process(KEYIN key, RawRecordContainer event, Multimap<String,NormalizedContentInterface> fields,
                        TaskInputOutputContext<KEYIN,? extends RawRecordContainer,KEYOUT,VALUEOUT> context, ContextWriter<KEYOUT,VALUEOUT> contextWriter)
                        throws IOException, InterruptedException {
-            long edgesCreated = 0;


this.deleteMode (above) is never used. I'm wondering if MyDerivedProtobufEdgeDataTypeHandler could be removed.

matthpeterson · 2024-02-07T16:01:33Z

...t-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java


-        trimFieldGroup = context.getConfiguration().getBoolean(typeName + TRIM_FIELD_GROUP, false);
+        // Track metadata for this event
+        eventMetadataRegistry = new HashMap<>();


I suggest initializing this at its declaration and clearing the Map each time process is called, to avoid having to create a new HashMap in each call to process.

matthpeterson · 2024-02-07T16:02:40Z

...t-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

-        loadDateStr = getLoadDateString(fields);
-
+        boolean trimGroups = context.getConfiguration().getBoolean(typeName + EdgeIngestConfiguration.TRIM_FIELD_GROUP, false);
+        EdgeEventFieldUtil edgeEventFieldUtil = new EdgeEventFieldUtil(trimGroups);


Also to avoid object creation and collection, I would consider creating a reusable EdgeEventFieldUtil and clearing it here instead of creating a new one.

Perhaps this suggestion is overkill. Either way, with the refactor it is probably worth observing performance impact. It would be cool if we had a JMH test handy for the datatype handler that would give us microbenchmarking information for process, given that it's called so much.

matthpeterson · 2024-02-07T16:08:06Z

...t-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

+        postProcessEdges(event, context, contextWriter, edgesCreated, baseEdgeBundle);
+
+        return edgesCreated;
+    }


Much cleaner, thank you!

matthpeterson · 2024-02-07T16:15:57Z

...use/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/EdgeEventFieldUtil.java

+    }
+
+    public EdgeDataBundle setEdgeInfoFromEventFields(EdgeDataBundle bundle, EdgeDefinitionConfigurationHelper edgeDefConfigs, RawRecordContainer event,
+                    EdgeIngestConfiguration edgeConfig, long newFormatStartDate, Configuration conf, String typeName) {


typeName is unused. Perhaps it should be?

matthpeterson · 2024-02-07T16:17:43Z

...use/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/EdgeEventFieldUtil.java

+        bundle.setDateType(dateType);
+    }
+
+    public EdgeDataBundle setEdgeInfoFromEventFields(EdgeDataBundle bundle, EdgeDefinitionConfigurationHelper edgeDefConfigs, RawRecordContainer event,


This method returns the bundle it received by reference. The return value itself is never used. Perhaps the method should just create a new bundle from the provided event and drop the first parameter.

matthpeterson · 2024-02-07T16:20:41Z

...use/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/EdgeEventFieldUtil.java

+        bundle.setEdgeAttribute2(getEdgeAttr2(edgeDefConfigs));
+        bundle.setEdgeAttribute3(getEdgeAttr3(edgeDefConfigs));
+        bundle.setRequiresMasking(event.isRequiresMasking());
+        bundle.setHelper(event.getDataType().getIngestHelper(conf));


Looking for opportunities to drop more method parameters - it looks like the conf parameter is only used here. I suggest updating EdgeIngestConfiguration to save its constructor argument (which is the same as this conf, retrieved from context.getConfiguration()), so that it's only necessary to use EdgeIngestConfiguration.

matthpeterson · 2024-02-07T16:23:32Z

...use/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/EdgeEventFieldUtil.java

+import datawave.ingest.time.Now;
+import datawave.util.time.DateHelper;
+
+public class EdgeEventFieldUtil {


I really like this class. I wonder if the name could be improved, mostly because Util raises some flags in my mind - but naming is hard so honestly I'm not sure what's better. Its primary behavior is to extract edge information from fields. So maybe FieldToEdgeDataGenerator or EdgeFieldExtractor. Feel free to ignore.

Conflicts: warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

hlgp and others added 13 commits November 14, 2022 23:38

Add edge precondition logic for group aware definitions

9a8d377

Handle more cases for edge preconditions

de38a51

Merge branch 'integration' into bugfix/edgePrecondition

4e89cdd

null check

6311862

Merge branch 'bugfix/edgePrecondition' of https://github.com/National…

cd78073

…SecurityAgency/datawave into bugfix/edgePrecondition

Address MR comments

5608491

Merge branch 'integration' into bugfix/edgePrecondition

51ab6a7

Merge branch 'integration' into task/refactorProtoHandler

8aad28a

address #1776 refactor ProtobufedgeDTH

36d143c

continuing refactor for #1776

400056f

Merge branch 'integration' into task/refactorProtoHandler

dd3e021

Conflicts: warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

Merge branch 'integration' into task/refactorProtoHandler

368a35e

Merge branch 'integration' into task/refactorProtoHandler

073e044

matthpeterson reviewed Feb 27, 2023

View reviewed changes

...t-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java Outdated Show resolved Hide resolved

matthpeterson reviewed Feb 27, 2023

View reviewed changes

...t-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java Outdated Show resolved Hide resolved

matthpeterson reviewed Feb 27, 2023

View reviewed changes

...t-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java Outdated Show resolved Hide resolved

matthpeterson reviewed Feb 27, 2023

View reviewed changes

...t-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java Outdated Show resolved Hide resolved

matthpeterson and others added 13 commits February 28, 2023 12:30

Update ProtobufEdgeDataTypeHandler.java (#1866)

2c41571

continuing refactor for #1776

c152296

Merge branch 'task/refactorProtoHandler' of https://github.com/Nation…

c291044

…alSecurityAgency/datawave into task/refactorProtoHandler

mid refactor funk

fd7dc1b

Continuing to break this out into specialized classes

0259cb8

Missed a builder attribute

34652fa

Validate more parts of the edge

48924cb

Merge branch 'integration' into task/refactorProtoHandler

f2799e8

Conflicts: warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

more tests for edge handler refactor

9d17fbe

More edge tests for refactor

fc2edea

Merge branch 'integration' into task/refactorProtoHandler

de863eb

hlgp and others added 2 commits January 12, 2024 17:32

Merge branch 'integration' into task/refactorProtoHandler

533becb

hlgp changed the title ~~WIP: Task/refactor proto handler~~ Task/refactor proto handler Jan 24, 2024

hlgp added 2 commits January 24, 2024 18:26

Merge branch 'integration' into task/refactorProtoHandler

247ff81

Merge branch 'integration' into task/refactorProtoHandler

eab765a

matthpeterson reviewed Feb 7, 2024

View reviewed changes

matthpeterson approved these changes Feb 7, 2024

View reviewed changes

hlgp and others added 4 commits September 16, 2024 15:08

Merge branch 'integration' into task/refactorProtoHandler

8eb451b

Conflicts: warehouse/ingest-core/src/main/java/datawave/ingest/mapreduce/handler/edge/ProtobufEdgeDataTypeHandler.java

Merge branch 'integration' into task/refactorProtoHandler

e77bae7

short circuit edges earlier

4c852cd

Merge branch 'integration' into task/refactorProtoHandler

b151093

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task/refactor proto handler #1830

Task/refactor proto handler #1830

hlgp commented Jan 31, 2023

matthpeterson Feb 7, 2024 •

edited

Loading

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

matthpeterson Feb 7, 2024

		@@ -97,6 +99,17 @@ public EdgeDataBundle(EdgeDefinition edgeDef, NormalizedContentInterface ifaceSo
		this.initMarkings(getSource().getMarkings(), getSink().getMarkings());

		@@ -983,28 +623,6 @@ private NormalizedContentInterface getNullKeyedNCI(String fieldValue, Multimap<S
		return null;

	this.edgeType = edgeDef.getEdgeType().toString();
	this.edgeType = edgeDef.getEdgeType();

Task/refactor proto handler #1830

Are you sure you want to change the base?

Task/refactor proto handler #1830

Conversation

hlgp commented Jan 31, 2023

matthpeterson Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthpeterson Feb 7, 2024 •

edited

Loading