search

logzio / sawmill

Sawmill is a JSON transformation Java library
Apache License 2.0
116 stars 24 forks source link

Add ExternalMappingSourceProcessor #271

Closed matvey-mtn closed 2 years ago

matvey-mtn commented 2 years ago

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

DanMelman commented 2 years ago

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

Missed that test. this is indeed enough coverage.

DanMelman commented 2 years ago

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

Which test is that? Still can't find it

matvey-mtn commented 2 years ago

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

Which test is that? Still can't find it

@DanMelman all tests that trigger processor.process(doc) will trigger refreshExternalMapping() on the first invocation.

DanMelman commented 2 years ago

Also, I would add a test for the regular mapping refresh. This might require changing the parameter from seconds to milliseconds to spare some test time.

@DanMelman I don't think we need this test. refreshExternalMapping() is already covered, and testing the ScheduledExecutorService doesn't make sense.

Which test is that? Still can't find it

@DanMelman all tests that trigger processor.process(doc) will trigger refreshExternalMapping() on the first invocation.

@matvey-mtn My intention was to test the map update/replacement, including the case where a reload failed and we need to validate the old map stayed

matvey-mtn commented 2 years ago

Which test is that? Still can't find it

@DanMelman all tests that trigger processor.process(doc) will trigger refreshExternalMapping() on the first invocation.

@matvey-mtn My intention was to test the map update/replacement, including the case where a reload failed and we need to validate the old map stayed

@DanMelman ok, I will add something